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

[React Refresh] support typescript namespace syntax #22621

Merged
merged 2 commits into from
Nov 9, 2021

Conversation

irinakk
Copy link
Contributor

@irinakk irinakk commented Oct 25, 2021

Summary

This resolves #22413 by adding support for typescript namespace syntax.

The test in ReactFreshBabelPlugin-test.js demonstrates the cause of the issue, which before the fix, the output code will be:

namespace Foo {
  export namespace Bar {
    export const Child = () => {};
    _c = Child;
  }
}

and after

namespace Foo {
  export namespace Bar {
    export const Child = () => {};
    _c = Child;
  }
}

var _c;
$RefreshReg$(_c, "Foo$Bar$Child");

As i explained in this comment, because of the wrong programPath, named function exports in namespaces got registered but never used. This error is exposed in vite because vite uses <script module> by default, and modules use strict mode automatically, and it treats the undeclared _c an error.

How did you test this change?

  • test added in ReactFreshBabelPlugin-test.js tests whether react-refresh/babel supports ts namespace syntax.
  • test added in ReactFreshIntegration-test.js mimics the work flow of how vite uses react-refresh/babel with typescript. the test would fail without this fix:

截屏2021-10-27 上午1 03 53

@sizebot
Copy link

sizebot commented Oct 25, 2021

Comparing: 4298ddb...e8052d8

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.19 kB 130.19 kB = 41.42 kB 41.41 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 135.16 kB 135.16 kB = 42.95 kB 42.95 kB
facebook-www/ReactDOM-prod.classic.js = 419.90 kB 419.90 kB = 77.34 kB 77.34 kB
facebook-www/ReactDOM-prod.modern.js = 408.48 kB 408.48 kB = 75.59 kB 75.59 kB
facebook-www/ReactDOMForked-prod.classic.js = 419.90 kB 419.90 kB = 77.34 kB 77.34 kB
oss-experimental/react-refresh/cjs/react-refresh-babel.production.min.js +8.73% 7.48 kB 8.13 kB +3.46% 2.66 kB 2.75 kB
oss-stable-semver/react-refresh/cjs/react-refresh-babel.production.min.js +8.73% 7.48 kB 8.13 kB +3.46% 2.66 kB 2.75 kB
oss-stable/react-refresh/cjs/react-refresh-babel.production.min.js +8.73% 7.48 kB 8.13 kB +3.46% 2.66 kB 2.75 kB
oss-experimental/react-refresh/cjs/react-refresh-babel.development.js +7.43% 24.97 kB 26.83 kB +4.41% 5.71 kB 5.96 kB
oss-stable-semver/react-refresh/cjs/react-refresh-babel.development.js +7.43% 24.97 kB 26.83 kB +4.41% 5.71 kB 5.96 kB
oss-stable/react-refresh/cjs/react-refresh-babel.development.js +7.43% 24.97 kB 26.83 kB +4.41% 5.71 kB 5.96 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 +8.73% 7.48 kB 8.13 kB +3.46% 2.66 kB 2.75 kB
oss-stable-semver/react-refresh/cjs/react-refresh-babel.production.min.js +8.73% 7.48 kB 8.13 kB +3.46% 2.66 kB 2.75 kB
oss-stable/react-refresh/cjs/react-refresh-babel.production.min.js +8.73% 7.48 kB 8.13 kB +3.46% 2.66 kB 2.75 kB
oss-experimental/react-refresh/cjs/react-refresh-babel.development.js +7.43% 24.97 kB 26.83 kB +4.41% 5.71 kB 5.96 kB
oss-stable-semver/react-refresh/cjs/react-refresh-babel.development.js +7.43% 24.97 kB 26.83 kB +4.41% 5.71 kB 5.96 kB
oss-stable/react-refresh/cjs/react-refresh-babel.development.js +7.43% 24.97 kB 26.83 kB +4.41% 5.71 kB 5.96 kB

Generated by 🚫 dangerJS against e8052d8

@gaearon
Copy link
Collaborator

gaearon commented Oct 26, 2021

Thanks for the PR! I wonder if we should handle namespaces though so that Fast Refresh works with them?

@irinakk
Copy link
Contributor Author

irinakk commented Oct 26, 2021

@gaearon yes that makes sense, i was thinking of the IIFE output but organising components in namespace seems a recommended practice in typescript + react. I'll see if i can make it happen.

edit: i changed the implementation to do the support rather than ignore, its more complicated with nested namespace then i thought. changed to draft and will reopen this when the pr is ready.

@irinakk irinakk force-pushed the fix/ignore-ts-namespace branch from 32c83df to f940f4b Compare October 26, 2021 04:58
@irinakk irinakk changed the title [React Refresh] ignore typescript namespace syntax [React Refresh] support typescript namespace syntax Oct 26, 2021
@irinakk irinakk marked this pull request as draft October 26, 2021 05:12
@irinakk irinakk marked this pull request as ready for review October 26, 2021 09:11
@irinakk irinakk force-pushed the fix/ignore-ts-namespace branch from e8052d8 to 003b251 Compare October 28, 2021 10:51
@gaearon
Copy link
Collaborator

gaearon commented Oct 28, 2021

This is looking really good from the first glance. Will look in more depth later today.

@gaearon gaearon merged commit ff9897d into facebook:main Nov 9, 2021
@irinakk irinakk deleted the fix/ignore-ts-namespace branch November 10, 2021 02:09
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Nov 15, 2021
Summary:
This sync includes the following changes:
- **[c0c71a868](facebook/react@c0c71a868 )**: Re-enable useMutableSource in internal RN ([#22750](facebook/react#22750)) //<Ricky>//
- **[cb11155c8](facebook/react@cb11155c8 )**: Add runtime type checks around module boundary code ([#22748](facebook/react#22748)) //<Brian Vaughn>//
- **[a04f13d29](facebook/react@a04f13d29 )**: react-refresh@0.11.0 //<Dan Abramov>//
- **[ff9897d23](facebook/react@ff9897d23 )**: [React Refresh] support typescript namespace syntax ([#22621](facebook/react#22621)) //<irinakk>//
- **[0ddd69d12](facebook/react@0ddd69d12 )**: Throw on hydration mismatch and force client rendering if boundary hasn't suspended within concurrent root ([#22629](facebook/react#22629)) //<salazarm>//
- **[c3f34e4be](facebook/react@c3f34e4be )**: eslint-plugin-react-hooks@4.3.0 //<Dan Abramov>//
- **[827021c4e](facebook/react@827021c4e )**: Changelog for eslint-plugin-react-hooks@4.3.0 //<Dan Abramov>//
- **[8ca3f567b](facebook/react@8ca3f567b )**: Fix module-boundary wrappers ([#22688](facebook/react#22688)) //<Brian Vaughn>//
- **[1bf6deb86](facebook/react@1bf6deb86 )**: Renamed packages/react-devtools-scheduling-profiler to packages/react-devtools-timeline ([#22691](facebook/react#22691)) //<Brian Vaughn>//
- **[51c558aeb](facebook/react@51c558aeb )**: Rename (some) "scheduling profiler" references to "timeline" ([#22690](facebook/react#22690)) //<Brian Vaughn>//
- **[00ced1e2b](facebook/react@00ced1e2b )**: Fix useId in strict mode ([#22681](facebook/react#22681)) //<Andrew Clark>//
- **[5cccacd13](facebook/react@5cccacd13 )**: Upgrade useId to alpha channel ([#22674](facebook/react#22674)) //<Andrew Clark>//
- **[75f3ddebf](facebook/react@75f3ddebf )**: Remove experimental useOpaqueIdentifier API ([#22672](facebook/react#22672)) //<Andrew Clark>//
- **[8c4a05b8f](facebook/react@8c4a05b8f )**: Remove flow pragma comment from module registration start/stop templates ([#22670](facebook/react#22670)) //<Brian Vaughn>//
- **[ebf9ae857](facebook/react@ebf9ae857 )**: useId ([#22644](facebook/react#22644)) //<Andrew Clark>//
- **[a0d991fe6](facebook/react@a0d991fe6 )**: Re-land #22292 (remove uMS from open source build) ([#22664](facebook/react#22664)) //<Andrew Clark>//
- **[6bce0355c](facebook/react@6bce0355c )**: Upgrade useSyncExternalStore to alpha channel ([#22662](facebook/react#22662)) //<Andrew Clark>//
- **[7034408ff](facebook/react@7034408ff )**: Follow-up improvements to error code extraction infra ([#22516](facebook/react#22516)) //<Andrew Clark>//
- **[90e5d3638](facebook/react@90e5d3638 )**: chore: fix comment typo ([#22615](facebook/react#22615)) //<btea>//
- **[3c4c1c470](facebook/react@3c4c1c470 )**: Remove warning for dangling passive effects ([#22609](facebook/react#22609)) //<Andrew Clark>//
- **[d5b6b4b86](facebook/react@d5b6b4b86 )**: Expand act warning to cover all APIs that might schedule React work ([#22607](facebook/react#22607)) //<Andrew Clark>//
- **[fa9bea0c4](facebook/react@fa9bea0c4 )**: Initial implementation of cache cleanup ([#22510](facebook/react#22510)) //<Joseph Savona>//
- **[0e8a5aff3](facebook/react@0e8a5aff3 )**: Scheduling Profiler: Add marks for component effects (mount and unmount) ([#22578](facebook/react#22578)) //<Brian Vaughn>//
- **[4ba20579d](facebook/react@4ba20579d )**: Scheduling Profiler: De-emphasize React internal frames ([#22588](facebook/react#22588)) //<Brian Vaughn>//
- **[cdb8a1d19](facebook/react@cdb8a1d19 )**: [Fizz] Add option to inject bootstrapping script tags after the shell is injected ([#22594](facebook/react#22594)) //<Sebastian Markbåge>//
- **[34e4c9756](facebook/react@34e4c9756 )**: Clear extra nodes if there's a hydration mismatch within a suspense boundary  ([#22592](facebook/react#22592)) //<Sebastian Markbåge>//
- **[02f411578](facebook/react@02f411578 )**: Upgrade useInsertionEffect to stable ([#22589](facebook/react#22589)) //<Andrew Clark>//
- **[2af4a7933](facebook/react@2af4a7933 )**: Hydrate using SuspenseComponent as the parent ([#22582](facebook/react#22582)) //<Sebastian Markbåge>//
- **[b1acff0cc](facebook/react@b1acff0cc )**: Enable cache in test renderer ([#22580](facebook/react#22580)) //<Joseph Savona>//
- **[996da67b2](facebook/react@996da67b2 )**: Replace global `jest` heuristic with `IS_REACT_ACT_ENVIRONMENT` ([#22562](facebook/react#22562)) //<Andrew Clark>//
- **[163e81c1f](facebook/react@163e81c1f )**: Support disabling spurious act warnings with a global environment flag ([#22561](facebook/react#22561)) //<Andrew Clark>//
- **[23b7dfeff](facebook/react@23b7dfeff )**: Enable scheduling profiler for RN FB profiling builds ([#22566](facebook/react#22566)) //<Brian Vaughn>//
- **[61455a25b](facebook/react@61455a25b )**: Enable experimental Cache API in www TestRenderer ([#22554](facebook/react#22554)) //<Joseph Savona>//
- **[7142d110b](facebook/react@7142d110b )**: Bugfix: Nested useOpaqueIdentifier references ([#22553](facebook/react#22553)) //<Andrew Clark>//
- **[1e247ff89](facebook/react@1e247ff89 )**: Enabled scheduling profiler marks for React Native FB target ([#22544](facebook/react#22544)) //<Brian Vaughn>//
- **[c16b005f2](facebook/react@c16b005f2 )**: Update test and stack frame code to support newer V8 stack formats ([#22477](facebook/react#22477)) //<Brian Vaughn>//
- **[55d75005b](facebook/react@55d75005b )**: duplicate value in variable ([#22390](facebook/react#22390)) //<BIKI DAS>//

Changelog:
[General][Changed] - React Native sync for revisions afcb9cd...c0c71a8

jest_e2e[run_all_tests]

Reviewed By: yungsters

Differential Revision: D32395873

fbshipit-source-id: 3afd158f167b1eedcc244e29aba1a2c502d3c9d9
zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Apr 15, 2022
* [React Refresh] support typescript namespace syntax

* [React Refresh] handle nested namespace

Co-authored-by: Wang Yilin <wang_yil@worksap.co.jp>
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.

Bug: Components inside typescript namespaces cause ReferenceError
4 participants