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: fix sourcemap when using object as define value #15805

Merged

Conversation

hi-ogawa
Copy link
Collaborator

@hi-ogawa hi-ogawa commented Feb 5, 2024

Description

I wrote my findings in #15771 (comment). The issue seems to be esbuild including multiple sources in source map when it cannot replace "define" value with primitive (esbuild example) while later pipeline for source map collapsing/remapping doesn't expect such extra sources entries.

Since it doesn't look possible to control how esbuild output source entries, I think one way to "fix" this issue is to simply remove all extra source map entries which doesn't correspond to the original source. In this PR, I attempted this approach by using utilities from @jridgewell/trace-mapping (which is mostly re-exports of @jridgewell/sourcemap-codec).

todo

  • test
    • rollup build
    • ssr dev

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, especially the Pull Request Guidelines.
  • 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).
  • Update the corresponding documentation if needed.
  • Ideally, include relevant tests that fail without this PR but pass with it.

Copy link

stackblitz bot commented Feb 5, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@hi-ogawa hi-ogawa changed the title fix: fix sourcemap when esbuild "define" transform includes extra entries fix: fix sourcemap when using object as define value Feb 5, 2024
@hi-ogawa hi-ogawa marked this pull request as ready for review February 5, 2024 06:16
sapphi-red
sapphi-red previously approved these changes Apr 3, 2024
Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Would you resolve the conflicts?

@sapphi-red sapphi-red added p3-minor-bug An edge case that only affects very specific usage (priority) feat: sourcemap Sourcemap support labels Apr 3, 2024
return index === sourceIndex
}),
)
result.map = JSON.stringify(encodedMap(new TraceMap(decoded as any)))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wasn't the case before, but now it looks like this is type error. I used as any for now.

@sapphi-red
Copy link
Member

/ecosystem-ci run

@patak-dev patak-dev merged commit 445c4f2 into vitejs:main Apr 3, 2024
10 checks passed
@hi-ogawa hi-ogawa deleted the fix-esbuild-define-transform-sourcemap branch April 3, 2024 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: sourcemap Sourcemap support 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.

sourcemap is invalid when environment variables do not exist
3 participants