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

Refactor codebase to TypeScript #264

Merged
merged 75 commits into from
Apr 11, 2020

Conversation

taras
Copy link
Contributor

@taras taras commented Mar 18, 2020

Note: I created an integration package that plugin developers can use to test this new package before it's published. Please, test it by installing @frontside/ink@next and let me know how it goes. I'll deprecate this package after this branch is merged.

Motivation

I refactored the codebase to TypeScript to make it easier to replace existing DOM with JSDOM and expose JSDOM for testing in more complex UI scenarios. This could also provide a path to #151.

Backstory

My team at Frontside and I are starting to work on the CLI for our new BigTest test runner. We wanted to use Ink but we wanted the CLI UI that could compete with Cypress' web UI.

To test that kind of Ink UI, we'll want to have the ability to assert on a DOM like tree. Ink has DOM like representation but it's not a real DOM so we can't use existing DOM traversal tools to select elements to assert on. I started looking at replacing Ink's DOM with JSDOM but I found it very difficult to navigate the code because it's not typed.

So, I wanted to add types before diving further.

Approach

I had to make quite a few changes.

  1. Added typescript@3.8.3
  2. Split up Ink instance into 2 separate constructors because they use different DOM structures.
  3. Typed all of the files
  4. Added types to yoga-layout-prebuilt and applied types everywhere it's used
  5. Changed xo to use typescript
  6. Upgraded Ava to 3.5.0 because I needed debugging functionality and all ava documentation for typescript references the new version
  7. BREAKING: change Node requirements to 10 & 12
  8. Configured GitHub Actions to use matrix of Node 10|12 & Experimental true|false
  9. Updated tests to typescript (but didn't type them completely)
  10. I used ts-node for tests but used version 7.0.0 of ts-node because it seems to have a bug in new versions that makes it slow.
  11. Removed the static index.d.ts in favour of generated types
  12. Added src directory to the distribution
  13. Changed props for Color component to consume types from Chalk

Questions/TODO

  • Can we remove PropTypes in favour of just using TypeScript?
  • Does this need to be a major release because of the Node dependency change?
  • Do you want me to revert to default or can we used named imports?
  • How should we ignore intentional unnecessary curly braces? Refactor codebase to TypeScript #264 (comment)

@taras taras mentioned this pull request Mar 18, 2020
@taras
Copy link
Contributor Author

taras commented Mar 18, 2020

BTW, tests are passing on my fork but are not running here https://github.com/taras/ink/actions/runs/58115570

package.json Outdated Show resolved Hide resolved
@taras
Copy link
Contributor Author

taras commented Mar 18, 2020

I squashed the commits. I'll start working on replacing DOM with JSDOM in another branch as I wait for feedback on this PR. No rush.

src/components/Color.tsx Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Collaborator

https://github.com/vadimdemedes/ink/blob/master/index.d.ts includes a lot of doc comments, can you ensure they're correct moved to the TS source?

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/build-layout.ts Outdated Show resolved Hide resolved
test/flex-justify-content.tsx Outdated Show resolved Hide resolved
.eslintignore Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/components/Color.tsx Outdated Show resolved Hide resolved
src/ink.tsx Outdated Show resolved Hide resolved
src/styles.ts Show resolved Hide resolved
test/components.tsx Outdated Show resolved Hide resolved
tsconfig.json Show resolved Hide resolved
test/render.tsx Show resolved Hide resolved
taras and others added 13 commits March 21, 2020 04:33
Co-Authored-By: Sindre Sorhus <sindresorhus@gmail.com>
Co-Authored-By: Sindre Sorhus <sindresorhus@gmail.com>
Co-Authored-By: Sindre Sorhus <sindresorhus@gmail.com>
Co-Authored-By: Sindre Sorhus <sindresorhus@gmail.com>
Co-Authored-By: Sindre Sorhus <sindresorhus@gmail.com>
Co-Authored-By: Sindre Sorhus <sindresorhus@gmail.com>
nice one

Co-Authored-By: Sindre Sorhus <sindresorhus@gmail.com>
@taras
Copy link
Contributor Author

taras commented Mar 28, 2020

@vadimdemedes do you want to keep the prop types? IOW, can I remove them?

@taras
Copy link
Contributor Author

taras commented Apr 5, 2020

@vadimdemedes I believe everything in this branch is done based on feedback.

@vadimdemedes
Copy link
Owner

Fantastic work, @taras, thank you! There are some tiny styling places to fix up, but I'll do that myself after this PR is merged. Considering the tremendous amount of change here, I'm going to take this opportunity to create a major release, including some other changes I wanted to make for a long time.

@vadimdemedes vadimdemedes merged commit c9631c2 into vadimdemedes:master Apr 11, 2020
@vadimdemedes
Copy link
Owner

Btw, is there a reason to include src folder in npm package?

@vadimdemedes
Copy link
Owner

typescript is in dependencies of package.json, should it be in dev dependencies instead?

@sindresorhus
Copy link
Collaborator

Btw, is there a reason to include src folder in npm package?

No

typescript is in dependencies of package.json, should it be in dev dependencies instead?

Yes

@vadimdemedes
Copy link
Owner

Cool, fixed both in master.

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.

4 participants