ember-cli / broccoli-terser-sourcemap

Broccoli filter to uglify with sourcemaps
MIT License
10 stars 35 forks source link

fix: Fix sourcemap file lookup #313

Closed mydea closed 1 year ago

mydea commented 1 year ago

We ran into this here: https://github.com/getsentry/sentry-javascript/issues/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 commented 1 year ago

@Turbo87 @rwjblue sorry for the ping, not sure who's the most actively maintaining this repo. Would be great to get this or https://github.com/ember-cli/broccoli-terser-sourcemap/pull/314 merged

Turbo87 commented 1 year ago

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

NullVoxPopuli commented 1 year ago

thanks a bunch for working on this! :tada:

mydea commented 1 year ago

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"!