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 codes to typescript and yarn to pnpm #45

Open
wants to merge 2 commits into
base: rewrite-media-styles
Choose a base branch
from

Conversation

tkow
Copy link
Contributor

@tkow tkow commented Dec 31, 2022

This repository almost doesn't depend on native library, so you can treat it as simple node_modules.
So, I try DX improved migrate some tools.

  1. yarn to pnpm because pnpm is speedy about clean installation and installation with lock file and cache (benchmark)
  2. update versions of test and babel tools
  3. use @swc/jest instead of 'babel-jest' from react-native/jest-preset.js. The former is very faster than the latter.Please check benchmark. (e.g.: es5 so on)

And there are additional points worth noting.

  1. I can replace react-native-bob-build to webpack and swc-loader that accelerates build speed, but I didn't because swc is undergoing developing and there are some deferent behaviors from tsc or babel-typescript we can't fix although most codes work properly, so it is safety to use babel or tsc now. The swc is introduced for high-performance test.
  2. The reason why can be safe in swc test codes in this repository is that codes of this library doesn't need babel-metro-preset, thus it correctly transpile them and the code is used only in the test environment.
  3. Expo example project keep yarn, because if you setup a pnpm react-native project, it needs more complex setting to adapt metro-bundler or webpack. (Please check this article).
  4. I added ts-jest, because your jest configuration or set up files can be written using typescript though it isn't used for transpliation.
  5. I don't still replace ambient files in types directories and it's used as primary exported types although I can do so. That means codes in internal emits js codes and sourcemap without type definitions as before but you can use typescript under development. If the current definitions depend on and need to follow @stitiches/react types, we migrate writing them manually to emitting them from tsc, but it's enough now to define and use type advantage in internal. So I want to do that after you will have finished working at https://github.com/Temzasse/stitches-native/tree/rewrite-media-styles because you may feel convenient to use typescript and test improvements in the branch with merging this PR soon. This is done though loosely typing. But, the logic is almost same first JS file and if these types are wrong, the types are not used in the users' environment.

@tkow
Copy link
Contributor Author

tkow commented Dec 31, 2022

@Temzasse

I can migrate them to typescript.

I marked @ts-nocheck now. Please review it and could you merge it after checking it and unmarking draft?
I'll try to remove @ts-nocheck as much as possible until you have finished reviewing.

update

I could remove all @ts-nocheck partial codes are loosely typed, and I had to change two js logics logic though it
affects nothing because I just only add logic to check undefined. It was not checked whether it's undefined but, it must be existed value as before.

https://github.com/Temzasse/stitches-native/pull/45/files#diff-a8848e503173984c129a0807b96c24aa84357346d46b6df7fbe57c7e9bf5a9f6R133

https://github.com/Temzasse/stitches-native/pull/45/files#diff-a8848e503173984c129a0807b96c24aa84357346d46b6df7fbe57c7e9bf5a9f6R114-R120

@tkow tkow force-pushed the rewrite-media-styles branch from 856541c to cbe59cc Compare December 31, 2022 09:00
@tkow tkow marked this pull request as ready for review December 31, 2022 09:05
@tkow tkow force-pushed the rewrite-media-styles branch 2 times, most recently from 29bdfb1 to b7deca1 Compare December 31, 2022 09:59
Copy link
Owner

@Temzasse Temzasse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the improvement suggestions! 👍🏻

I think it would be easier to review and merge these changes if we split them into two different PRs: one with the config changes (SWC/Jest stuff) and another with the TS changes in the internals.

I added some initial comments about the config changes but didn't yet have the time to look at the TS changes.

'react-native-svg',
].join('|');

export default {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little hesitant about switching to SWC since the current babel based setup works quite well and the time it takes to execute all the tests is only like 5 seconds so it doesn't take that long any way 🤔

Copy link
Contributor Author

@tkow tkow Dec 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Temzasse
Of course, I get lit of it, if you don't like it.
But, let me explain it to you a little, the configuration is very simple and easy to replace. All you need to do is @swc/jest to babel-jest if wanting to switch it, and it marks 5 times faster than babel-jest in my project's test which takes more than 3 minutes if using babel-jest or ts-jest. In my local environment of this project, it passed test less than 2 sec (1.77 s, estimated 2 s), (0.863 s, estimated 1 s with cache).
The case that SWC is fragile is when you compress and mangle codes (these features aren't needed if only you want to test) and with using decorator's metadata, and vercel is already using it in their library, Next.js. so in almost case you don't need to worry about it if you don't use conifgurations described above.

But, you don't feel pain to follow up this tool, I'll remove it.

@@ -0,0 +1,31 @@
const esmModules = [
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed? We don't use any of these libs in the internals of the library.

Copy link
Contributor Author

@tkow tkow Dec 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is recommended configuration introduced by expo.

https://docs.expo.dev/guides/testing-with-jest/#configuration

If we use pnpm, the module resolution system is a bit different from npm and it caches the modules in node_modules/.pnpm, so need manually setting at least, '@react-native/' namespace. See react-native preset.

It's not high possibility to use another native-library, but it's ready for testing with third party libraries. If you don't need them, I remove them except for first react-native configuration (first array element configuration is
in high possibility to be required for test-libraries though react-native's jest preset narrower than it) which needs transpiled.

"metro-react-native-babel-preset": "^0.73.3",
"npm-watch": "^0.11.0",
"prettier": "2.3.2",
"react": "18.0.0",
"react-native": "0.69.6",
"react-native-builder-bob": "0.18.1",
"react-test-renderer": "18.0.0",
"ts-jest": "^29.0.3",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are ts-jest and ts-node needed since .tsx files are transpiled by @swc/jest?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, they aren't.
The ts-jest is used for jest.config.ts. If you install it, you can write jest config files in typescript like jest-setup.ts, jest-environement.ts so on, and specify ts files in the jest.config file. ts-node is set peerDependency in ts-jest.@swc/jest itself can run without ts-jest and ts-node.

package.json Outdated
"react-native-builder-bob": {
"source": "src/internals",
"output": "lib",
"targets": [
"commonjs",
"module"
"module",
"typescript"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't want to let builder bob generate the type defs since we want to have the types be defined separately since it's practically impossible to get the type defs work correctly by inferring them from the internals.

Copy link
Contributor Author

@tkow tkow Dec 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry this is my mistake.
I met test error at first after setting minimum configuration, and I thought this configuration needed. But, Only I needed was to rename index.ts to index.tsx to fix it. I'll remove it.

fix: remove yarn
@tkow tkow force-pushed the rewrite-media-styles branch from b7deca1 to 6e132b5 Compare December 31, 2022 12:37
@tkow
Copy link
Contributor Author

tkow commented Dec 31, 2022

@Temzasse

Now, this PR includes SWC configuraiton and only rename js files to ts files with @ts-ignore to prevent js codes from being broken.

@tkow tkow mentioned this pull request Dec 31, 2022
@tkow
Copy link
Contributor Author

tkow commented Dec 31, 2022

Furthermore I notice we need to modify github actions.

  1. I switched yarn to pnpm in ci
  2. Change how to run tests in ci. They run only after PR branch have been created, though pushing all branches everytime triggers before.

And https://github.com/Temzasse/stitches-native/actions/runs/3812284842/jobs/6485411228 shows it works fine.

with:
node-version: '15'
Copy link
Contributor Author

@tkow tkow Jan 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is changed to fix even number versioning for LTS, because odd number versioning of node is experimental and generally for development.
https://nodesource.com/blog/understanding-how-node-js-release-lines-work/

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.

2 participants