Skip to content

fix: fix sourcemap when using object as define value #15805

Merged
patak-cat merged 17 commits intovitejs:mainfrom
hi-ogawa:fix-esbuild-define-transform-sourcemap
Apr 3, 2024
Merged

fix: fix sourcemap when using object as define value #15805
patak-cat merged 17 commits intovitejs:mainfrom
hi-ogawa:fix-esbuild-define-transform-sourcemap

Conversation

@hi-ogawa
Copy link
Contributor

@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.

@bolt-new-by-stackblitz
Copy link

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
Contributor 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-cat patak-cat merged commit 445c4f2 into vitejs:main Apr 3, 2024
@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