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(react): sourcemap incorrect warning and classic runtime sourcemap #9006

Merged

Conversation

sapphi-red
Copy link
Member

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

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
Copy link

netlify bot commented Jul 9, 2022

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

packages/plugin-react/src/index.ts Outdated Show resolved Hide resolved
@sapphi-red sapphi-red changed the title fix(react): sourcemap incorrect warning fix(react): sourcemap incorrect warning and classic runtime sourcemap Jul 9, 2022
packages/plugin-react/src/index.ts Outdated Show resolved Hide resolved
@bluwy
Copy link
Member

bluwy commented Jul 9, 2022

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
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
Copy link
Member

bluwy commented Jul 9, 2022

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.

bluwy
bluwy approved these changes Jul 9, 2022
@patak-dev
Copy link
Member

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

@patak-dev patak-dev merged commit bdae7fa into vitejs:main Jul 9, 2022
12 checks passed
@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.

None yet

3 participants