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

Sourcemap mismatch for unpkg/UMD build #1041

Open
chrisdjali-wrld3d opened this issue Apr 27, 2023 · 8 comments · May be fixed by #1043
Open

Sourcemap mismatch for unpkg/UMD build #1041

chrisdjali-wrld3d opened this issue Apr 27, 2023 · 8 comments · May be fixed by #1043

Comments

@chrisdjali-wrld3d
Copy link

When building wrld.js for CDN consumption, either by passing --format umd to the microbundle command, or calling microbundle without arguments so it uses the "unpkg" entry in package.json, the source map produced contains incorrect offsets. The source maps produced for the "main", "exports" and "module" builds work properly, though.

As far as I can tell, the issue can be worked around by adding forty four whitespace characters to the start of the emitted script, although I've not confirmed whether that works for everything or just the few things I tried. I don't see an obvious reason why there'd be this offset or why 44 is the number of extra characters that magically cancels it out.

@rschristian
Copy link
Collaborator

Odd. Just to make sure I'm reproducing correctly, you're running build:cdn but with source maps enabled instead, correct?

@chrisdjali-wrld3d
Copy link
Author

watch:cdn and build:package (which just calls microbundle) both create cdn/wrld.js and a broken cdn/wrld.js.map, but yes, it would be the same as getting rid of --sourcemap false from build:cdn.

@rschristian
Copy link
Collaborator

Have any way to reproduce? Naively trying to load your script from the instructions in your ReadMe has shown no issues in the source map, though your script will proceed to absolutely devour my CPU, maxing it out and locking up Firefox to the point where I'll need to kill the process. I'm not too keen on blindly testing any further.

@chrisdjali-wrld3d
Copy link
Author

The only thing I can think of that might be different if you build the package is that we're stuck on a fairly old node (16.10.0) and npm (6.14.15) version. The actual dependencies you end up with should be the same as we've got a package-lock.json checked in.

As for it locking up your browser, that's not something we've seen before, so we'd ideally want to get to the bottom of that before it affects anyone who actually wants to use Wrld. What precisely did you do?

Anyway, the problem should be immediately visible if you load the compiled UMD module and its source map in a source map viewer, so it shouldn't be necessary to actually run it.

@rschristian
Copy link
Collaborator

The only thing I can think of that might be different if you build the package is that we're stuck on a fairly old node (16.10.0) and npm (6.14.15) version.

Building on different Node versions wouldn't have any effect that I'm aware of, though coincidentally I am on Node 16 w/ and have NPM v6 lying around too.

As for it locking up your browser, that's not something we've seen before, so we'd ideally want to get to the bottom of that before it affects anyone who actually wants to use Wrld. What precisely did you do?

Precisely? Don't recall, didn't keep it around. Copy/pasted from the first example in your docs with a null, undefined, or empty string for an API key (I forget which). One of those options may cause issues, don't know. Feel free to assume it was user error of course.

Anyway, the problem should be immediately visible if you load the compiled UMD module and its source map in a source map viewer, so it shouldn't be necessary to actually run it.

While something like this indeed has shown them to be off, I couldn't actually reproduce an issue in a browser. I inserted a number of exceptions into the lib and in all my tests, the browser could point to the right location with the source map. Is this an actual issue you've had in consuming these source maps, or is this purely by testing with a viewer?

@chrisdjali-wrld3d
Copy link
Author

Yes, I initially noticed the issue when consuming the source maps in Chrome. I'm usually working on C++, so am used to optimising compilers making stack traces bear little resemblance to what you'd get with optimisations disabled, but I noticed things were consistently off by a few lines. That's when I looked into source map viewers, and they were consistent with what I'm seeing. E.g. if I set a breakpoint on a particular statement in the built module, it'll end up showing in the mapped source in the location the source map viewer indicated it would.

@rschristian
Copy link
Collaborator

rschristian commented May 2, 2023

Looks like this is reproducible with a much smaller input:

export function Foo() {
    return 'Foo';
}

As you say, a 44 space offset in the generated file "works". Will look into it

@rschristian
Copy link
Collaborator

Found it:

microbundle/src/index.js

Lines 651 to 668 in 105a09b

// Rollup 2 injects globalThis, which is nice, but doesn't really make sense for Microbundle.
// Only ESM environments necessitate globalThis, and UMD bundles can't be properly loaded as ESM.
// So we remove the globalThis check, replacing it with `this||self` to match Rollup 1's output:
renderChunk(code, chunk, opts) {
if (opts.format === 'umd') {
// minified:
code = code.replace(
/([a-zA-Z$_]+)="undefined"!=typeof globalThis\?globalThis:(\1\|\|self)/,
'$2',
);
// unminified:
code = code.replace(
/(global *= *)typeof +globalThis *!== *['"]undefined['"] *\? *globalThis *: *(global *\|\| *self)/,
'$1$2',
);
return { code, map: null };
}
},

We replace globalThis in UMD bundles, but don't correct the sourcemaps afterwards. Will try to get a fix together later.

@rschristian rschristian linked a pull request May 6, 2023 that will close this issue
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 a pull request may close this issue.

2 participants