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

fix: support typescript namespace #22601

Closed
wants to merge 3 commits into from
Closed

fix: support typescript namespace #22601

wants to merge 3 commits into from

Conversation

ideffix
Copy link

@ideffix ideffix commented Oct 20, 2021

Summary

This PR is possible fix for #22413. It solves bug with namespace and vite. Related issue.

How did you test this change?

I created a simple app like mentioned in #22413 and then I applied the same changes directly in node_modules of this app. This change is solving mentioned issue.

@gaearon
Copy link
Collaborator

gaearon commented Oct 20, 2021

Can you add a regression test? So, a test that fails without this change.

@sizebot
Copy link

sizebot commented Oct 20, 2021

Comparing: 996da67...2839452

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 130.15 kB 130.15 kB = 41.41 kB 41.41 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 132.98 kB 132.98 kB = 42.39 kB 42.39 kB
facebook-www/ReactDOM-prod.classic.js = 414.32 kB 414.32 kB = 76.58 kB 76.57 kB
facebook-www/ReactDOM-prod.modern.js = 402.91 kB 402.91 kB = 74.85 kB 74.85 kB
facebook-www/ReactDOMForked-prod.classic.js = 414.32 kB 414.32 kB = 76.58 kB 76.58 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-experimental/react-refresh/cjs/react-refresh-babel.production.min.js +0.79% 7.48 kB 7.54 kB +1.13% 2.66 kB 2.69 kB
oss-stable-semver/react-refresh/cjs/react-refresh-babel.production.min.js +0.79% 7.48 kB 7.54 kB +1.13% 2.66 kB 2.69 kB
oss-stable/react-refresh/cjs/react-refresh-babel.production.min.js +0.79% 7.48 kB 7.54 kB +1.13% 2.66 kB 2.69 kB
oss-experimental/react-refresh/cjs/react-refresh-babel.development.js +0.40% 24.97 kB 25.07 kB +0.54% 5.71 kB 5.74 kB
oss-stable-semver/react-refresh/cjs/react-refresh-babel.development.js +0.40% 24.97 kB 25.07 kB +0.54% 5.71 kB 5.74 kB
oss-stable/react-refresh/cjs/react-refresh-babel.development.js +0.40% 24.97 kB 25.07 kB +0.54% 5.71 kB 5.74 kB

Generated by 🚫 dangerJS against 2839452

@ideffix
Copy link
Author

ideffix commented Oct 21, 2021

@gaearon during my invastigation in source code of vite I noticed that in order to create code that is failing it do following things:

  1. Transpile code with babel (this step is using react-refresh/babel.js)
  2. Compile it with esbuild
    This code is presenting simplified version of this process.

After first step there is still TS code. It is being compiled in second step.
So in order to reproduce (aka write regression test) I would need to add esbuild to compile TS code but that creates tight coupling with typescript and I'm not sure if that is something that we want in source code.

@owen-kosman
Copy link

@ideffix I tried running your changes on the issue example and the ReferenceError still appears. I followed instructions from https://reactjs.org/docs/how-to-contribute.html to try your changes locally (npm). I might have done it wrongly, is this the right way to test your change locally?

Writing a failing test and passing it confers highest confidence for the changes you did. From what I understand, Babel transpiles directly from TS to JS, so you shouldn't expect any remaining TS code in there (https://www.typescriptlang.org/docs/handbook/babel-with-typescript.html). esbuild is not related to this issue as far as I understand it, so we shouldn't need it in integration or unit tests.

@ideffix
Copy link
Author

ideffix commented Oct 23, 2021

@owen-kosman in order to check this locally follow these steps:

  1. In react root folder run npm run build (it will take a few moments, is there a way to build single package?)
  2. Go to build/node_modules/react-refresh
  3. Link this module (Personally I use yalc so yalc publish)
  4. Go to your vite app and link module from previous step yalc add react-refresh.
  5. npm run dev

About babel and TS - yes it transpiles TS code into JS but only if you add relevant plugins/presets. According to source code of vite it is using babel with react-refresh plugin but without TS plugins/presets so after this step there is still Typescript code which is being transpiled later in a process using esbuild.

So in order to reporduce this issue in integration test - I needed to transpile it in 2 steps:

  1. babel + react-refresh plugin
  2. babel + TS preset

If we merge this babel configs into one this error will not occur (it works as expected) so in my integration test I just reproduce steps that vite is doing.

@irinakk
Copy link
Contributor

irinakk commented Oct 24, 2021

@ideffix hi, i think this bug is about the plugin doesn't understand typescript syntax, so its not related to transform typescript. no need for adding @babel/preset-typescript, @babel/plugin-syntax-typescript would be sufficient.
I made a fix based on my comment here, i'd like to made a pr if this doesn't offend you.

@ideffix
Copy link
Author

ideffix commented Oct 24, 2021

hi @irinakk , sure I would love to see your fix 😃

@gaearon
Copy link
Collaborator

gaearon commented Nov 9, 2021

This was fixed in #22621.

@gaearon gaearon closed this Nov 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants