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: update sourcemap in importAnalysisBuild #7825

Merged

Conversation

sapphi-red
Copy link
Member

@sapphi-red sapphi-red commented Apr 20, 2022

Description

importAnalysisBuild was breaking sourcemaps when there is a dynamic import.

This is a reproduction.
https://stackblitz.com/edit/vitejs-vite-ipkqha?file=main.js&terminal=dev

import './style.css';

console.log('1');
import('./dy');
console.log('2');
/* omit below */
  • Current v2.9.5.
    image
  • With this PR (The output 2 has changed from line 4 to 5)
    image

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 bug p3-minor-bug An edge case that only affects very specific usage (priority) labels Apr 20, 2022
bluwy
bluwy previously approved these changes Apr 20, 2022
}

// there may still be markers due to inlined dynamic imports, remove
// all the markers regardless
chunk.code = chunk.code.replace(preloadMarkerRE, 'void 0')
const s = new MagicString(chunk.code)
Copy link
Member

Choose a reason for hiding this comment

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

Could we create this MagicString at the top so we only use one, and only need to combine the source maps once?

Copy link
Member Author

Choose a reason for hiding this comment

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

It needs to replace markers which were not replaced. Using the same MagicString instance was doing a different behavior here.

import MagicString from 'magic-string';

const mark = '__VITE_PRELOAD__';

const code = `
  import('foo.js', "__VITE_PRELOAD__")
  "__VITE_PRELOAD__"
`;

const s = new MagicString(code);

s.overwrite(
  code.indexOf(mark) - 1,
  code.indexOf(mark) + mark.length + 1,
  '["deps.js"]',
  {
    contentOnly: true,
  }
);

/*
  import('foo.js', ["deps.js"])
  "__VITE_PRELOAD__"
*/
console.log(s.toString());

s.replace(new RegExp(`"${mark}"`, 'g'), 'void 0');

/*
  import('foo.js', void 0)
  void 0
*/
console.log(s.toString());

To change this, it will need to store positions at line 355 and then do the replace without s.replace at line 375 with skipping the stored positions.
Should I do so? (It's going to be easier than I first thought.)

Copy link
Member

Choose a reason for hiding this comment

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

To change this, it will need to store positions at line 355 and then do the replace without s.replace at line 375 with skipping the stored positions.
Should I do so? (It's going to be easier than I first thought.)

Sounds like a good thing to do now that I re-look the code, should improve perf too.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have changed it 👍

Co-authored-by: patak <matias.capeletto@gmail.com>
@patak-dev patak-dev merged commit d7540c8 into vitejs:main Apr 21, 2022
@sapphi-red sapphi-red deleted the fix/update-sourcemap-import-analysis-build branch April 21, 2022 18:13
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