-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 source maps in published packages #20850
Conversation
I noticed this problem from this warning in my build: ``` Warning: ignoring input sourcemap for ../rewritten-packages/@embroider/synthesized-vendor/vendor/ember/ember-testing.js because ENOENT: no such file or directory, open '.../node_modules/.embroider/rewritten-packages/@embroider/synthesized-vendor/vendor/ember/ember-template-compiler.js.map' ``` Indeed, it seems like as of 5.10.0-beta.1, we stopped publishing source maps: https://unpkg.com/browse/ember-source@5.10.0-beta.1/dist/ ...despite having every intention to do so still. Last good version: https://unpkg.com/browse/ember-source@5.10.0-alpha.4/dist/ It took me way to long to spot this, but the problem is in the extension; in the broccoli build, they had a `$name.map` filename, whereas in the new build (#20675) it's `$name.js.map`. Adjusted the entries in package.json to match; also removed the types publish script that was accidentally included in the tarball. * * * As for my original quest to fix the warning, it turns out this is not sufficient. The file is placed there by @embroider/compat these days: embroider-build/embroider#2164 It'll have to be updated to also emit the source map file (if it is present, since this has been broken for a while). But this is a prerequisite to fixing that.
d3e73b0
to
7b0a083
Compare
@@ -25,16 +25,16 @@ | |||
"dist/packages", | |||
"dist/dependencies", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this one is also broken. Last good version is:
https://unpkg.com/browse/ember-source@5.10.0-alpha.4/dist/dependencies/
If it's been broken for so long and nobody complained, I'm not sure if it needs to be brought back. There are a lot of code in the build scripts to try and support this, but clearly something isn't working. I don't care about those and did not investigate why.
If we want to remove it, it would take some effort to read through the build code to find and clean up the defunct code for this, so I'm leaving that out of scope for this PR for someone else to decide/pick up.
I'm also not sure how important these source maps are and if it warrants a LTS backport, so someone else would have to decide that. I think sources maps, in general, are important, but I don't know what, if anything, actually uses these |
"docs/data.json", | ||
"lib", | ||
"types" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why'd this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair! Thank you!
Thanks, this PR looks good.
This ember bundles in dist are used by:
With the exception of ember-template-compiler.js which is still used by pretty much everybody. |
I noticed this problem from this warning in my build:
Indeed, it seems like as of 5.10.0-beta.1, we stopped publishing source maps:
https://unpkg.com/browse/ember-source@5.10.0-beta.1/dist/
...despite having every intention to do so still.
Last good version:
https://unpkg.com/browse/ember-source@5.10.0-alpha.4/dist/
It took me way to long to spot this, but the problem is in the extension; in the broccoli build, they had a
$name.map
filename, whereas in the new build (#20675) it's$name.js.map
.Adjusted the entries in package.json to match; also removed the types publish script that was accidentally included in the tarball.
As for my original quest to fix the warning, it turns out this is not sufficient. The file is placed there by @embroider/compat these days:
embroider-build/embroider#2164
It'll have to be updated to also emit the source map file (if it is present, since this has been broken for a while).
But this is a prerequisite to fixing that.