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

perf(sourcemaps): add back lazy sourcemap decode and handling nullish mappings #5087

Merged

Conversation

thebanjomatic
Copy link
Contributor

@thebanjomatic thebanjomatic commented Aug 4, 2023

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:

resolves #5084

Description

We weren't properly handling ill-formed (according to types) sourcemap input such as {mappings: null} {mappings: undefined} or {}. This PR expands the set of values that can be passed in to decodedSourcemaps to ensure that we are handling all those edge cases and doing something sane. In this case, we treat mappings: null the same as mappings: '' and return the "empty sourcemap"

return {
  mappings: [],
  names: [],
  sources: [],
  version: 3
};

@vercel
Copy link

vercel bot commented Aug 4, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
rollup ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 8, 2023 5:08pm

@thebanjomatic thebanjomatic changed the title Perf/lazy sourcemap decode perf(sourcemaps): add back lazy sourcemap decode and handling nullish mappings Aug 4, 2023
@codecov
Copy link

codecov bot commented Aug 4, 2023

Codecov Report

Merging #5087 (80b2d26) into master (6787f44) will decrease coverage by 0.01%.
Report is 6 commits behind head on master.
The diff coverage is 94.44%.

@@            Coverage Diff             @@
##           master    #5087      +/-   ##
==========================================
- Coverage   98.97%   98.96%   -0.01%     
==========================================
  Files         226      226              
  Lines        8362     8390      +28     
  Branches     2294     2303       +9     
==========================================
+ Hits         8276     8303      +27     
  Misses         31       31              
- Partials       55       56       +1     
Files Changed Coverage Δ
src/utils/decodedSourcemap.ts 93.54% <92.59%> (+7.83%) ⬆️
src/Module.ts 100.00% <100.00%> (ø)
src/utils/collapseSourcemaps.ts 90.62% <100.00%> (+0.19%) ⬆️
src/utils/getOriginalLocation.ts 100.00% <100.00%> (ø)

@thebanjomatic
Copy link
Contributor Author

@lukastaegert Sorry to ping you, but can this be merged and re-released if the change looks good? Bug fix is in the second commit to make it clearer what the problem was, but feel free to squash and merge if you'd like.

@lukastaegert
Copy link
Member

Ah sorry, thanks for reminding me, I will take care.

@lukastaegert lukastaegert merged commit a1d9930 into rollup:master Aug 9, 2023
10 of 12 checks passed
@rollup-bot
Copy link
Collaborator

This PR has been released as part of rollup@3.28.0. You can test it via npm install rollup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeError: Cannot read properties of undefined (reading 'indexOf')
3 participants