vite icon indicating copy to clipboard operation
vite copied to clipboard

fix(plugin-react): duplicate __self prop and __source prop

Open Dunqing opened this issue 3 years ago • 6 comments

Description

fix: #9363

Both __source and __self are automatically set when using the automatic runtime.

So we should avoid transform a ReactElement containing __source or __self props to jsx.

Additional context


What is the purpose of this pull request?

  • [x] Bug fix
  • [ ] New Feature
  • [ ] Documentation update
  • [ ] Other

Before submitting the PR, please make sure you do the following

  • [x] Read the Contributing Guidelines.
  • [x] Read the Pull Request Guidelines and follow the Commit Convention.
  • [x] Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • [x] Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • [x] Ideally, include relevant tests that fail without this PR but pass with it.

Dunqing avatar Jul 27 '22 02:07 Dunqing

Would you explain why this was happening only when NODE_ENV=development?

Also IMO we should remove __source and __self after transformed to JSX instead of avoiding the transform.

sapphi-red avatar Jul 27 '22 05:07 sapphi-red

Would you explain why this was happening only when NODE_ENV=development?

https://github.com/babel/babel/blob/029fa1778296ad2e854313d2582aacf3a0d74d43/packages/babel-plugin-transform-react-jsx/src/create-plugin.ts#L564-L573

Because the __self and __source props are automatically added only when NODE_ENV=development.

Also IMO we should remove __source and __self after transformed to JSX instead of avoiding the transform.

Not quite agree, we should not remove the user-defined __source or __self and use the auto-generated one, we should make it consistent

Dunqing avatar Jul 27 '22 06:07 Dunqing

I did some research, I think the root of the problem is sentry, which is probably using an older bundler that causes the npm source code to contain __self and __source props, so we just need to get it to run successfully.

Dunqing avatar Jul 27 '22 06:07 Dunqing

Because the __self and __source props are automatically added only when NODE_ENV=development.

I did some research, I think the root of the problem is sentry, which is probably using an older bundler that causes the npm source code to contain __self and __source props, we just need to get it to run successfully.

@sentry/react's bundle includes __source/__self regardless of NODE_ENV and babel-plugin-transform-react-jsx injects them when NODE_ENV=development. So it's only duplicated when NODE_ENV=development. Is my understanding correct?

Not quite agree, we should not remove the user-defined __source or __self and use the auto-generated one, we should make it consistent

I think this is a build consistency / development info / runtime performance trade-off.

  • avoid transform with both NODE_ENV=development and NODE_ENV=prodcution: __source/__self is correct but we cannot gain performance of automatic runtime
  • avoid transform with NODE_ENV=development but trasform NODE_ENV=prodcution: __source/__self is correct but build between NODE_ENV is not consistent
  • transform with both NODE_ENV=development and NODE_ENV=prodcution: __source/__self is not correct

sapphi-red avatar Jul 27 '22 06:07 sapphi-red

About the runtime perf, I'm not sure how much it affects. https://github.com/alloc/vite-react-jsx#faq

sapphi-red avatar Jul 27 '22 07:07 sapphi-red

@sentry/react's bundle includes __source/__self regardless of NODE_ENV and babel-plugin-transform-react-jsx injects them when NODE_ENV=development. So it's only duplicated when NODE_ENV=development. Is my understanding correct?

Exactly!

I think this is a build consistency / development info / runtime performance trade-off.

  • avoid transform with both NODE_ENV=development and NODE_ENV=prodcution: __source/__self is correct but we cannot gain performance of automatic runtime
  • avoid transform with NODE_ENV=development but trasform NODE_ENV=prodcution: __source/__self is correct but build between NODE_ENV is not consistent
  • transform with both NODE_ENV=development and NODE_ENV=prodcution: __source/__self is not correct

I think there are very few cases where a bundle contains __self or __source, so we just need to ignore the cases that contain __self or __source. This shouldn't have much of an impact.

It is worth mentioning that before #9386, I found that most of the ReactElements were not converted to jsx.🤣

Dunqing avatar Jul 27 '22 07:07 Dunqing

@aleclarson Thank you, Can you review this?

Dunqing avatar Aug 03 '22 11:08 Dunqing

I am hoping this PR will be merged. I am having the same problem with a combination of Ladle and react-bootstrap.

aki77 avatar Aug 25 '22 22:08 aki77