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(build): let top-level this refer to globalThis #5312

Merged
merged 2 commits into from Oct 21, 2021

Conversation

aleclarson
Copy link
Member

@aleclarson aleclarson commented Oct 15, 2021

Description

This prevents THIS_IS_UNDEFINED warning from Rollup when @babel/plugin-transform-react-jsx emits jsx calls that pass this inside an arrow function component, which in turn leads to SOURCEMAP_ERROR warnings, because the this reference doesn't exist in the original code.

An example Rollup warning message for SOURCEMAP_ERROR:

Error when using sourcemap for reporting an error: Can't resolve original location of error.

How code output is changed

This PR is especially important for @vitejs/plugin-react.

  return /* @__PURE__ */ jsxDevRuntime.exports.jsxDEV(Provider, {
    value: props,
    children
  }, void 0, false, {
    fileName: _jsxFileName$z,
    lineNumber: 42,
    columnNumber: 26
  }, this); // <== The problem

…becomes…

  return /* @__PURE__ */ jsxDevRuntime.exports.jsxDEV(Provider, {
    value: props,
    children
  }, void 0, false, {
    fileName: _jsxFileName$z,
    lineNumber: 42,
    columnNumber: 26
  }, window); // <== The solution

Workaround

Ignore the warning, or set rollupOptions.onwarn to handle it manually.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

This prevents THIS_IS_UNDEFINED warning from Rollup when @babel/plugin-transform-react-jsx emits `jsx` calls that pass `this` inside an arrow function component, which in turn leads to SOURCEMAP_ERROR warnings, because the `this` reference doesn't exist in the original code.
@aleclarson aleclarson added the p2-edge-case Bug, but has workaround or limited in scope (priority) label Oct 15, 2021
@aleclarson
Copy link
Member Author

Hmm, I wonder if we should use "globalThis" in SSR env? @patak-js

@patak-dev
Copy link
Member

@aleclarson I think we should use globalThis for non-legacy. It is supported from Node 12. The legacy plugin should take of transpiling it down to window when necessary, no?

@jordanamr
Copy link

Is there a way to silence this whilst waiting on this PR merge ? This is spamming my build logs with thousands of lines ahah

@aleclarson
Copy link
Member Author

@jordanamr Set rollupOptions.context to "window" or "globalThis" in your Vite config

@antfu
Copy link
Member

antfu commented Oct 18, 2021

I would vote for globalThis

packages/vite/src/node/build.ts Outdated Show resolved Hide resolved
Co-authored-by: patak <matias.capeletto@gmail.com>
@aleclarson aleclarson changed the title fix: let top-level this refer to window fix: let top-level this refer to globalThis Oct 20, 2021
@aleclarson aleclarson changed the title fix: let top-level this refer to globalThis fix(build): let top-level this refer to globalThis Oct 20, 2021
@patak-dev patak-dev merged commit 7e25429 into vitejs:main Oct 21, 2021
@genffy
Copy link

genffy commented Nov 10, 2021

this fix cause anthoer issue, will convert inputPath with global keyword to gloalThis nuxt/nuxt#11958

@patak-dev
Copy link
Member

Could you open a new issue so this csn be properly tracked?

@genffy
Copy link

genffy commented Nov 10, 2021

Could you open a new issue so this csn be properly tracked?

sorry, comment metioned issue root casue is nuxt/bridge#204

@patak-dev
Copy link
Member

If you think there is something to fix in Vite, please open an issue here

@aleclarson
Copy link
Member Author

@genffy That issue is unrelated to this PR.

@vitejs vitejs locked and limited conversation to collaborators Nov 10, 2021
@aleclarson aleclarson deleted the fix/context-window branch February 25, 2022 20:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
p2-edge-case Bug, but has workaround or limited in scope (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants