Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate from Flow to Typescript #456 #475

Merged
merged 11 commits into from
Aug 5, 2023

Conversation

nateplusplus
Copy link
Contributor

Closes #456

This has been a fun challenge and a great TypeScript exercise! 😄

It would be great to get another pair of eyes on this work to ensure nothing was missed. This touches the entire library, so it is a pretty major change. I am happy to update any issues you find. I have also checked "Allow edits by maintainers" so feel free to add to my branch if necessary.

⚠️ Of note: I have not been able to test importing the TS version of this library via npm. It may be worth releasing a beta version to test thoroughly before the official release.

What was done:

  • Converted all Flow syntax to TS
  • Removed flow dependencies and files
  • Installed any necessary libraries for type definitions
  • Fixed any syntax that was not compatible with TypeScript
  • Converted all unit tests to TS and updated Jest configuration
  • Noted possible issues with TODO comments (will also create separate issues for each)
  • Added package scripts to run TS checks in testing and on deployment
  • Updated documentation, including CONTRIBUTING guide to reflect changes

To test

  1. Pull this working branch
  2. Run npm i to install dependencies in both the root folder and example/
  3. You should be able to run the example site locally as usual with npm start
  4. You can run typescript checks with the new script: npm run test:ts on the project root, and npm run typescript on the example site. Note: this will not emit any files, but will only check for errors.
  5. All unit tests should work as they do on the main branch (note: there are some warnings about not wrapping tests in act() but this is a pre-existing issue and not a result of the TypeScript changes.
  6. Building the project will compile into index.js and index.modern.mjs as usual, however you will also notice extra TypeScript definition files, and those will be exported in a directory structure which matches the src directory.

@nateplusplus
Copy link
Contributor Author

I saw the CI build failed, and it looks like it's due to a peer dependency conflict on the example site (react-scripts). Oddly enough I haven't seen that issue on my end, but to resolve it I have downgraded typescript to ^4 on the example site.

I see that the Flow stage is stuck in status: "Expected — Waiting for status to be reported". This might be due to branch rules set on the repository. Since Flow is no longer in the workflow, it will need to be removed.

@raymond-lam
Copy link
Collaborator

@nateplusplus This is amazing, thank you! We'll take a look at this PR soon!

@raymond-lam raymond-lam merged commit a61c4fc into ginkgobioworks:main Aug 5, 2023
12 checks passed
@wizztjh
Copy link

wizztjh commented Sep 14, 2023

Thanks for the work!
@raymond-lam Is it possible to bump the version v2.10.3 for it to be build?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate from Flow to Typescript
3 participants