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: Ensure identity source map has the same number of fields as the combined source map #122

Merged
merged 2 commits into from
Jul 22, 2023

Conversation

kota65535
Copy link
Contributor

@kota65535 kota65535 commented Jul 13, 2023

Hi @iFaxity,

In PR #115, we have removed the file field in the identity source map because I thought combined source maps from previous plugins do not have it. However, I found it is not always true.

The file field of a combined source map is added here in combineSourcemaps function. Below is the call tree from this.getCombinedSourcemap.

The combineSourcemaps function is not called if TransformContext#sourcemapChain is empty or has only 1 element.
So whether the file field exists or not depends on how many previous plugins have output the source maps to be combined, which is practically unpredictable.

The solution is quite simple. If the combined source map has a file field, the identity source map should.
Generally speaking, the two source maps should have the same number of optional fields (file, sourceRoot, and sourceContent) defined in Source Map Revision 3 format.

This PR has the following changes:

  • makes an identity source map use the same file and sourceRoot as the combined source map
  • ensures sourceContent is sanitized from an identity source map

@kota65535 kota65535 changed the title Fix: identity source map should have the same number of fields as the combined source map fix: Ensure identity source map has the same number of fields as the combined source map Jul 13, 2023
@kota65535 kota65535 marked this pull request as ready for review July 13, 2023 09:37
@iFaxity
Copy link
Owner

iFaxity commented Jul 22, 2023

LGTM! Merging!

Thanks again for contributing @kota65535.

@iFaxity iFaxity merged commit a75c1cc into iFaxity:next Jul 22, 2023
@github-actions
Copy link
Contributor

🎉 This PR is included in version 5.0.0-rc.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

iFaxity pushed a commit that referenced this pull request Jul 22, 2023
…combined source map (#122)

* fix: source map should have the same number of optional fields
* fix
iFaxity pushed a commit that referenced this pull request Jul 22, 2023
…combined source map (#122)

* fix: source map should have the same number of optional fields
* fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants