Skip to content

fix(react): sourcemap incorrect warning and classic runtime sourcemap#9006

Merged
patak-cat merged 5 commits into
vitejs:mainfrom
sapphi-red:fix/react-sourcemap-incorrect-warning
Jul 9, 2022
Merged

fix(react): sourcemap incorrect warning and classic runtime sourcemap#9006
patak-cat merged 5 commits into
vitejs:mainfrom
sapphi-red:fix/react-sourcemap-incorrect-warning

Conversation

@sapphi-red

@sapphi-red sapphi-red commented Jul 9, 2022

Copy link
Copy Markdown
Member

Description

From plugin-react 2.0.0-beta.1, the following warning was happening. This PR fixes that.

Sourcemap is likely to be incorrect: a plugin (vite:react-babel) was used to transform files, but didn't generate a sourcemap for the transformation. Consult the plugin documentation for help

Reproduction: https://stackblitz.com/edit/vitejs-vite-g4zebx

Also this PR fixes classic runtime sourcemap which was incorrect in some cases.

refs #8676

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

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

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

@sapphi-red sapphi-red added p3-minor-bug An edge case that only affects very specific usage (priority) plugin: react labels Jul 9, 2022
@netlify

netlify Bot commented Jul 9, 2022

Copy link
Copy Markdown

Deploy Preview for vite-docs-main canceled.

Name Link
🔨 Latest commit 6ae7d3f
🔍 Latest deploy log https://app.netlify.com/sites/vite-docs-main/deploys/62c99a98279d3000083ad072

Comment thread packages/plugin-react/src/index.ts Outdated
@sapphi-red sapphi-red changed the title fix(react): sourcemap incorrect warning fix(react): sourcemap incorrect warning and classic runtime sourcemap Jul 9, 2022
Comment thread packages/plugin-react/src/index.ts Outdated
@bluwy

bluwy commented Jul 9, 2022

Copy link
Copy Markdown
Member

Had a wild idea if we append the React import at the end instead so we don't have to generate sourcemaps, since ES import hoists, but that sounds dangerous (or is it) 😄

@sapphi-red

Copy link
Copy Markdown
Member Author

That's smart. I thought this might not work, but actually it seems to work.

// foo.mjs
console.log(foo) // 'foo'

await new Promise(resolve => setTimeout(resolve, 100))

console.log(foo) // 'foo'

import { foo } from './react.mjs'
// react.mjs
await new Promise(resolve => setTimeout(resolve, 100))

export const foo = 'foo'

It seems there is some issues when ESM -> CJS transform is used, but since we don't do that it won't be a issue?
microsoft/TypeScript#16166, swc-project/swc#1686, evanw/esbuild#1444

Another thing we may need to consider is sideeffects.

import './a.js' // depends on sideeffect of react but doesn't import react

If we prepend react, it works. If we append it, it doesn't. But I'm not sure if this needs to be considered.

@bluwy

bluwy commented Jul 9, 2022

Copy link
Copy Markdown
Member

Yeah I don't think we'd hit the ESM->CJS issue since the TypeScript's compiler isn't used in Vite. I think the trick should mostly work but cant tell for sure until we get some user-testing. I'd still be happy to proceed with this PR to be on the safe side though.

@patak-cat

Copy link
Copy Markdown
Member

We can explore this in v4, lets try to keep things stable now 😄

@patak-cat patak-cat merged commit bdae7fa into vitejs:main Jul 9, 2022
@sapphi-red sapphi-red deleted the fix/react-sourcemap-incorrect-warning branch July 10, 2022 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

p3-minor-bug An edge case that only affects very specific usage (priority)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants