Skip to content

Conversation

@maxvoltage
Copy link

@maxvoltage maxvoltage commented Jan 5, 2018

  • vendor.js file can have multiple sourceMappingURL locations when modules have their own sourceMappingURL. source-map-url module finds only one sourceMappingURL at a time. This PR removes sourceMappingURL if it doesn't have the same name with the js file until it finds the correct one.

  • Use the last map file if there is no map file with the same name with the js file. This is possibly wrong and it might rather be correct to raise an error in such case.

Related issue: ember-cli/ember-cli-terser#29

…file.

use the last map file if there is no map file with the same name with the js file. this is possibly wrong and it might rather be correct to raise an error in such case.
@ashleysommer
Copy link

ashleysommer commented Jan 11, 2018

This commit has a logic error.
The lastUrl variable actually stores the first found sourceMappingURL.
The variable is never updated during the loop.
If no matching filename is found in the file, it uses lastUrl, which still contains the first found sourceMappingURL.
I believe the intention was for lastUrl to be updating during the loop so it always contains the last found sourceMappingURL, so it can fallback to correctly to the last one when if a correct one is not found.

@ashleysommer
Copy link

ashleysommer commented Jan 11, 2018

This is the correct logic:

  let url = srcURL.getFrom(src);
  let lastUrl = srcURL.getFrom(src);
  // Make sure vendor.map is being used because vendor.js can have multiple sourceMappingURL if modules also have it.
  while (url && url.match(/.+?(?=\.map$)/)[0] !== relativePath.match(/(.*\/)?(.+)\.js/)[2]) {
    src = srcURL.removeFrom(src);
    lastUrl = url;    
    url = srcURL.getFrom(src);
  }
  // If there is no map url which has an identical name with the js file, use the last one.
  // It's possible we should rather raise an error instead.
  if (!url) {
    url = lastUrl;
  }

@maxvoltage
Copy link
Author

thanks @ashleysommer , you understood my intention correctly. I will update the PR.

let url = srcURL.getFrom(src);
let lastUrl = null;
// Make sure vendor.map is being used because vendor.js can have multiple sourceMappingURL if modules also have it.
while (url && url.match(/.+?(?=\.map$)/)[0] !== relativePath.match(/(.*\/)?(.+)\.js/)[2]) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

just a heads-up, tests will be required

@Turbo87
Copy link
Member

Turbo87 commented Jan 20, 2018

@maxvoltage sorry, I don't understand what problem exactly this PR is trying to fix. #55 should have fixed most of the issues. please make sure you're using the current version of this dependency.

@ashleysommer
Copy link

@Turbo87

#55 fix only applies when there is a single sourceMappingURL in the src file. It simply reads the directive, checks if the file exists, and ignores the sourceMap if it doesn't exist. It doesn't check for other sourceMappingURL directives in the src.

This fix applies in the case that there are two or more sourceMappingURL directives in the src (usually caused by concatenating multiple third-party libraries into a single vendor.js file).

This fix asserts that the most likely correct sourceMappingURL will be the one that matches/contains the filename of the src file.

Failing that, it falls back to using the last found sourceMappingURL, because the spec indicates that the correct sourceMappingURL should be at the end of the src file, and any before that must be artifacts from concatenation.

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.

5 participants