Migrate code into Typescript #411

Jianrong-Yu · 2022-05-17T04:25:27Z

Similar to #169, I tried to migrate everything to TypeScript in this PR.

Some changes:

Vite is replaced to tsc to generate both ESM and CommonJS package

vercel · 2022-05-17T04:25:29Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
react-stick	✅ Ready (Inspect)	Visit Preview	May 24, 2022 at 4:47PM (UTC)

codecov-commenter · 2022-05-17T04:49:47Z

Codecov Report

Merging #411 (8c6533d) into master (de83389) will decrease coverage by 5.16%.
The diff coverage is 91.17%.

@@            Coverage Diff             @@
##           master     #411      +/-   ##
==========================================
- Coverage   96.09%   90.93%   -5.17%     
==========================================
  Files          13       13              
  Lines         282      309      +27     
  Branches        0       92      +92     
==========================================
+ Hits          271      281      +10     
- Misses         11       17       +6     
- Partials        0       11      +11

Impacted Files	Coverage Δ
src/defaultPosition.ts	`100.00% <ø> (ø)`
src/utils/fit.ts	`100.00% <ø> (ø)`
src/utils/getDefaultAlign.ts	`100.00% <ø> (ø)`
src/utils/getModifiers.ts	`66.66% <ø> (ø)`
src/utils/scroll.ts	`50.00% <ø> (ø)`
src/utils/uniqueId.ts	`100.00% <ø> (ø)`
src/hooks/useAutoFlip.ts	`97.10% <80.00%> (ø)`
src/StickPortal.tsx	`83.72% <83.33%> (ø)`
src/Stick.tsx	`92.77% <100.00%> (ø)`
src/StickContext.ts	`100.00% <100.00%> (ø)`
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 191c0d6...8c6533d. Read the comment docs.

crecotun · 2022-05-17T07:33:40Z

Hi @Jianrong-Yu
Thank you for doing this.
Would you mind making sure this is going to be marked as Breaking Change and result in a major version update?

tzimmermann · 2022-05-17T07:45:41Z

Just published this as 5.0.0-rc1 under next tag on NPM: https://www.npmjs.com/package/react-stick/v/5.0.0-rc1
To test in your app, use npm install --save react-stick@next

tzimmermann · 2022-05-17T14:52:59Z

I've checked so far

Prettier & Eslint commit hooks are working as expected.
Usage of 5.0.0-rc1 in our app (pex-client) and in glucose works as expected.
Compared the sample HTML pages and could not spot any differences to master.

What I haven't checked:

All the JS->TS transformations (I am no TS expert). Does it even make sense to review them, or did you generate them with flowts @Jianrong-Yu ?

crecotun · 2022-05-17T15:14:15Z

I am not sure about replacing Vite with tsc.
My idea was to move our demos from webpack to Vite.
Vite does support TypeScript out of the box, so replacing the existing functionality simply doesn't bring any value.
I'd consider reverting this change.
What do you think @tzimmermann @Jianrong-Yu ?

.vscode/extensions.json

crecotun · 2022-05-17T17:46:03Z

.vscode/settings.json

-  "prettier.semi": false,
-  "prettier.singleQuote": true,
-  "prettier.trailingComma": "es5"
+  "editor.formatOnSave": true,


This one, ideally, too

You mean editor.formatOnSave one? Or prettier config moved to eslint?
We can remove formatOnSave if you don't like it. The format will be fixed by lint-staged when the code is committing anyway.

Sorry, I should provide more context here.

All the editor settings that you're adding will change the developer's IDE normal behavior and I usually tend to avoid doing this as everyone has their preferences.

What we can do and we already are doing — add some pre-commit hooks to ensure code passes eslint and prettier.

And it's not fair to add settings for vscode, and completely ignore WebStorm, for example. Having settings for all IDE will just pollute the project.

What do you think?

Sorry, I should provide more context here.

All the editor settings that you're adding will change the developer's IDE normal behavior and I usually tend to avoid doing this as everyone has their preferences.

What we can do and we already are doing — add some pre-commit hooks to ensure code passes eslint and prettier.

And it's not fair to add settings for vscode, and completely ignore WebStorm, for example. Having settings for all IDE will just pollute the project.

What do you think?

@crecotun I agree it's not fair to have settings for vscode only, but since vscode is almost the de facto editor in front-end community, I think it's good to have the vscode only settings. The Typescript is also opinionated, but it's also the de facto javascript static typing solution now: the developers are still able to use react-stick in Javascript, but they will get better support if they are using Typescript; just like users of VSCode will get more support, but the users of other editor will still get the formatting by Husky and Lint-staged in committing.
BTW, the vscode settings.json is already there before this PR, I just added a few new items in it.

Jianrong-Yu · 2022-05-17T18:35:22Z

What I haven't checked:

All the JS->TS transformations (I am no TS expert). Does it even make sense to review them, or did you generate them with flowts @Jianrong-Yu ?

Hi @tzimmermann Yes the type is guaranteed by flowts, the only change on type is the StickPropsT now have also all the attributes of an HTMLElement (like onMouseEnter and onMouseLeave used in demo/src/regressions/StickOnHover.tsx). It's because I found the ...rest always passed to the container:

    <Component
      {...rest}
      {...styles}

Hi @Jianrong-Yu Thank you for doing this. Would you mind making sure this is going to be marked as Breaking Change and result in a major version update?

Hi @crecotun The API didn't change, so from the aspect of users there is no breaking change for react-stick.

I am not sure about replacing Vite with tsc. My idea was to move our demos from webpack to Vite. Vite does support TypeScript out of the box, so replacing the existing functionality simply doesn't bring any value. I'd consider reverting this change. What do you think @tzimmermann @Jianrong-Yu ?

Bundling: I tried Typescript in Vite before I replaced it with tsc. Vite itself will not do the bundling for package; the bundling is done by rollup. To make the rollup working for typescript, plugin @rollup/plugin-typescript must be installed and put in the Vite configuration. Since it can be done just by using tsc, why choose another tool wrapped twice to do almose the same thing?
Testing: Testing must be done by Karma since it's the best choice for the unit-tests in a real browser. Jest is not working well in this case. And webpack with babel is the only combination I tried successfully on TypeScript/Coverage support in Karma, so webpack and babel (used in tests/karma.conf.js) are also required.
Demo: Since the webpack is required by testing, I don't think it's a good idea to migrate Demo into Vite. We always need webpack so why not reuse it in Demo?

crecotun · 2022-05-17T17:45:36Z

.vscode/extensions.json

@@ -0,0 +1,13 @@
+{


I believe this shouldn't end up in the codebase

crecotun · 2022-05-18T06:27:50Z

.vscode/settings.json

-  "prettier.semi": false,
-  "prettier.singleQuote": true,
-  "prettier.trailingComma": "es5"
+  "editor.formatOnSave": true,


Sorry, I should provide more context here.

All the editor settings that you're adding will change the developer's IDE normal behavior and I usually tend to avoid doing this as everyone has their preferences.

What we can do and we already are doing — add some pre-commit hooks to ensure code passes eslint and prettier.

And it's not fair to add settings for vscode, and completely ignore WebStorm, for example. Having settings for all IDE will just pollute the project.

What do you think?

crecotun · 2022-05-18T06:32:05Z

@Jianrong-Yu I wouldn't agree that for users there was no change, you replaced Flow with TypeScript, so end-user is affected and this is a major change. I might be wrong here.

Regarding the webpack.
Yes, it does make sense. I wasn't aware that for making it work with rollup we have to install an additional plugin.
So I'd say yes, we can live with webpack and tsc compiler as long as they do their job.

Testing.
One thing to mention here, I'd consider moving from Karma to Cypress + Jest if that makes sense. Cypress allows testing components in isolation.

Jianrong-Yu · 2022-05-18T16:30:53Z

@Jianrong-Yu I wouldn't agree that for users there was no change, you replaced Flow with TypeScript, so end-user is affected and this is a major change. I might be wrong here.

@crecotun Yes, I agree there are some breaking changes. The .flow files will not be generated anymore and old projects written in flowJs will lose the typing protection. It should be a major version update with breaking change notice for the flowjs users. Thanks @tzimmermann released it in beta tag.

Testing. One thing to mention here, I'd consider moving from Karma to Cypress + Jest if that makes sense. Cypress allows testing components in isolation.

It's a good idea to move the cases from Karma to Cypress (I'm not sure if Cypress is able to work with jest), Since it may changes the testing cases a lot, I didn't try it in this PR, we should try it later.

BREAKING CHANGE: FlowJS typing files changed to .d.ts files for TypeScript, flowjs is no supported from this commit

…nfiguration

Jianrong-Yu · 2022-05-23T23:44:19Z

@crecotun @tzimmermann commitlint is added in these new commits, can you do a review on them? Thanks.

BTW should I merge this PR once it's approved? The version number will be jump to 5.0 since it's a breaking change on typing.

crecotun · 2022-05-24T09:50:37Z

@Jianrong-Yu Could you explain why you added commitlint to the circleci?

Jianrong-Yu · 2022-05-24T16:03:24Z

@Jianrong-Yu Could you explain why you added commitlint to the circleci?

@crecotun I found all my old commits did't meet the requirements of semantic-release but it passed the circleCI check, I think it's better to have this check in CI so the developer will get notification for the comment if it's not met the requirement. The husky with lint-staged is useful for developers to push changes on local env but it cannot notify the user for any change done on Github web site directly.
If you think it's a bit overkill, I'll revert the change on circleCI.

crecotun · 2022-05-24T16:32:07Z

@Jianrong-Yu I'd rather keep this pr about TS only and if you don't mind, create another PR with commit lint improvements.
Would that work for you?

Jianrong-Yu · 2022-05-24T16:49:20Z

@Jianrong-Yu I'd rather keep this pr about TS only and if you don't mind, create another PR with commit lint improvements. Would that work for you?

@crecotun I agree we should avoid introducing new things into this PR. I've reset it to the commit before commitlint and did a force push. Sorry a new approval is required since the branch is updated 😂

pex-bot · 2022-05-24T16:56:06Z

🎉 This PR is included in version 5.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

vercel bot deployed to Preview May 17, 2022 04:48 View deployment

Jianrong-Yu requested a review from tzimmermann May 17, 2022 04:56

crecotun requested changes May 17, 2022

View reviewed changes

vercel bot deployed to Preview May 17, 2022 18:36 View deployment

signavio deleted a comment from crecotun May 17, 2022

Jianrong-Yu requested a review from crecotun May 17, 2022 18:41

crecotun reviewed May 18, 2022

View reviewed changes

crecotun previously approved these changes May 21, 2022

View reviewed changes

Jianrong-Yu dismissed crecotun’s stale review via 74de17f May 23, 2022 21:24

Jianrong-Yu force-pushed the migrate/typescript branch from 38db034 to 74de17f Compare May 23, 2022 21:24

Jianrong-Yu added 5 commits May 23, 2022 15:59

refactor: migrate from flowjs to Typescript

feab038

BREAKING CHANGE: FlowJS typing files changed to .d.ts files for TypeScript, flowjs is no supported from this commit

chore(linting): lint and fix all files by the same eslint/prettier co…

09656fd

…nfiguration

chore(circleci): remove flow from .circleci

c9c4c56

chore(linting): add husky and lint-staged

cd65684

chore(deps): clean package.json, remove unused dependencies

3695495

Jianrong-Yu force-pushed the migrate/typescript branch from 74de17f to 8c6533d Compare May 23, 2022 23:09

vercel bot deployed to Preview May 23, 2022 23:10 View deployment

Jianrong-Yu requested a review from crecotun May 23, 2022 23:41

Jianrong-Yu force-pushed the migrate/typescript branch from 8c6533d to 3695495 Compare May 24, 2022 16:46

vercel bot deployed to Preview May 24, 2022 16:47 View deployment

crecotun approved these changes May 24, 2022

View reviewed changes

Jianrong-Yu merged commit f5ce353 into master May 24, 2022

Jianrong-Yu deleted the migrate/typescript branch May 24, 2022 16:54

pex-bot added the released label May 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate code into Typescript #411

Migrate code into Typescript #411

Jianrong-Yu commented May 17, 2022

vercel bot commented May 17, 2022 •

edited

Loading

codecov-commenter commented May 17, 2022 •

edited

Loading

crecotun commented May 17, 2022

tzimmermann commented May 17, 2022

tzimmermann commented May 17, 2022

crecotun commented May 17, 2022

crecotun May 17, 2022

Jianrong-Yu May 17, 2022

crecotun May 18, 2022

Jianrong-Yu May 19, 2022

Jianrong-Yu commented May 17, 2022

crecotun May 17, 2022

crecotun May 18, 2022

crecotun commented May 18, 2022

Jianrong-Yu commented May 18, 2022 •

edited

Loading

Jianrong-Yu commented May 23, 2022

crecotun commented May 24, 2022

Jianrong-Yu commented May 24, 2022

crecotun commented May 24, 2022

Jianrong-Yu commented May 24, 2022

pex-bot commented May 24, 2022

Migrate code into Typescript #411

Migrate code into Typescript #411

Conversation

Jianrong-Yu commented May 17, 2022

vercel bot commented May 17, 2022 • edited Loading

codecov-commenter commented May 17, 2022 • edited Loading

Codecov Report

crecotun commented May 17, 2022

tzimmermann commented May 17, 2022

tzimmermann commented May 17, 2022

crecotun commented May 17, 2022

crecotun May 17, 2022

Choose a reason for hiding this comment

Jianrong-Yu May 17, 2022

Choose a reason for hiding this comment

crecotun May 18, 2022

Choose a reason for hiding this comment

Jianrong-Yu May 19, 2022

Choose a reason for hiding this comment

Jianrong-Yu commented May 17, 2022

crecotun May 17, 2022

Choose a reason for hiding this comment

crecotun May 18, 2022

Choose a reason for hiding this comment

crecotun commented May 18, 2022

Jianrong-Yu commented May 18, 2022 • edited Loading

Jianrong-Yu commented May 23, 2022

crecotun commented May 24, 2022

Jianrong-Yu commented May 24, 2022

crecotun commented May 24, 2022

Jianrong-Yu commented May 24, 2022

pex-bot commented May 24, 2022

vercel bot commented May 17, 2022 •

edited

Loading

codecov-commenter commented May 17, 2022 •

edited

Loading

Jianrong-Yu commented May 18, 2022 •

edited

Loading