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: Fix sourcemap file lookup #313

Merged
merged 1 commit into from Oct 31, 2023
Merged

Conversation

mydea
Copy link
Contributor

@mydea mydea commented Oct 24, 2023

We ran into this here: getsentry/sentry-javascript#9168, where the build failed when source maps & terser were enabled.

The root cause is that the used dependency: https://github.com/lydell/source-map-url is very old, and the regex it uses is not bullet proof. So this code (that some dependency generated):

function createURL(base64, sourcemapArg, enableUnicodeArg) {
    var sourcemap = sourcemapArg === undefined ? null : sourcemapArg;
    var enableUnicode = enableUnicodeArg === undefined ? false : enableUnicodeArg;
    var source = decodeBase64(base64, enableUnicode);
    var start = source.indexOf('\n', 10) + 1;
    var body = source.substring(start) + (sourcemap ? '//# sourceMappingURL=' + sourcemap : '');
    var blob = new Blob([body], { type: 'application/javascript' });
    return URL.createObjectURL(blob);
}

Was incorrectly matched by the regex, but return an '' url. Which then in turn messed up this addon, because while fs.existsSync(sourceMapPath) passed, it actually pointed to a directory, not a path (because of let sourceMapPath = path.join(inFileDirname, urls[i]);, where the url is '').

This PR does two thigns:

  1. Update to use the regex from https://github.com/thlorenz/convert-source-map, which is more up to date and also what source-map-url recommends
  2. Try-catch reading the file anyhow, so even if something goes wrong there, no need to blow up

@herzzanu
Copy link

@Turbo87 @rwjblue sorry for the ping, not sure who's the most actively maintaining this repo. Would be great to get this or #314 merged

@Turbo87
Copy link
Member

Turbo87 commented Oct 25, 2023

/cc @ember-cli/ember-cli ⬆️


// Forked from https://github.com/lydell/source-map-url/blob/master/source-map-url.js
// We use a slightly adjusted regex here
const convertSourceMap = require('convert-source-map');
Copy link
Contributor

Choose a reason for hiding this comment

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

any way we can use convert-source-map directly instead of needing teh lib/source-map-url.js file?

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 seems to work a bit differently, and I didn't find a good method it exposed (?) to just get all source map URLs in a file - there is a method to just get the last one, and a method to remove all of them in the file, both of which are not suited to what we need here 😓

@@ -16,6 +16,7 @@ Object {
background: white;
}",
"with-broken-sourcemap.js": "function meaningOfLife(){throw new Error(42)}",
"with-sourcemap-like-string.js": "function createURL(e,n,a){var i=void 0===n?null:n,t=decodeBase64(e,void 0!==a&&a),c=t.indexOf(\\"\\\\n\\",10)+1,r=t.substring(c)+(i?\\"//# sourceMappingURL=\\"+i:\\"\\"),o=new Blob([r],{type:\\"application/javascript\\"});return URL.createObjectURL(o)}function createURL2(e,n,a){var i=void 0===n?null:n,t=decodeBase64(e,void 0!==a&&a),c=t.indexOf(\\"\\\\n\\",10)+1,r=t.substring(c)+(i?\\"//# sourceMappingURL=\\"+i:\\"\\"),o=new Blob([r],{type:\\"application/javascript\\"});return URL.createObjectURL(o)}",
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks a ton for the additional tests!!!!

@@ -6089,7 +6094,7 @@ source-map-support@^0.5.6, source-map-support@~0.5.19:
buffer-from "^1.0.0"
source-map "^0.6.0"

source-map-url@^0.4.0, source-map-url@^0.4.1:
source-map-url@^0.4.0:
Copy link
Contributor

Choose a reason for hiding this comment

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

now that 0.4.1 is removed, what's still bringing this in to the dep graph? 🤔

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 is some deep dependency (source-map-resolve < snapdragon < nanomatch ....) 😬

@NullVoxPopuli
Copy link
Contributor

thanks a bunch for working on this! 🎉

Copy link
Contributor

@NullVoxPopuli NullVoxPopuli left a comment

Choose a reason for hiding this comment

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

Thanks a ton for working on this!!!

@mydea
Copy link
Contributor Author

mydea commented Oct 30, 2023

Thanks a ton for working on this!!!

🙏 it would be great if we could merge & release this as a patch level release in order for all packages depending on this getting this fix "for free"!

@ef4 ef4 merged commit 2e9bd88 into ember-cli:master Oct 31, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants