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

🐛 Insert empty newline as first line of minified outputs #37640

Merged
merged 9 commits into from Feb 11, 2022

Conversation

jridgewell
Copy link
Contributor

@jridgewell jridgewell commented Feb 11, 2022

Our sourcemaps got borked again by prepending code without updating the sourcemaps. To make this easy for both AMP_CONFIG and AMP_EXP, we're putting the ;\n back at the beginning of every file. Basically a revert of #36008. revert revert revert revert…

@amp-owners-bot
Copy link

Hey @rsimha! These files were changed:

build-system/common/esbuild-babel.js

@jridgewell jridgewell enabled auto-merge (squash) February 11, 2022 04:05
@samouri
Copy link
Member

samouri commented Feb 11, 2022

Our sourcemaps got borked again by prepending code without updating the sourcemaps.

Was this a recent change?

Copy link
Member

@danielrozenberg danielrozenberg left a comment

Choose a reason for hiding this comment

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

AMP_EXP gets dynamically injected into v0.[m]js - wouldn't that break the sourcemap too?

path: original.path,
text,
get contents() {
// eslint-disable-next-line local/no-forbidden-terms
Copy link
Member

Choose a reason for hiding this comment

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

Nit: add this as an exception in build-system/test-configs/forbidden-terms.js instead of an explicit eslint-disable here?

I don't know why I didn't do this originally, it's sooo much simpler. This has babel output an inline sourcemap, which is fed into esbuild. Esbuild will do an initial sourcemap-aware bundling, and output a new map that correctly points through babel into our real source files.
@jridgewell
Copy link
Contributor Author

AMP_EXP gets dynamically injected into v0.[m]js - wouldn't that break the sourcemap too?

Damn. I changed this back to using preamble to prepend a ;\n to the beginning of all files.

@jridgewell jridgewell changed the title 🐛 Make prepending AMP_CONFIG sourcemap aware 🐛 Insert empty newline as first line of minified outputs Feb 11, 2022
@samouri
Copy link
Member

samouri commented Feb 11, 2022

AMP_EXP gets dynamically injected into v0.[m]js - wouldn't that break the sourcemap too?

Where does this happen? In AMP Cache? In that case, there is no way around the semicolon hack

@jridgewell
Copy link
Contributor Author

The unversioned JS content transformer.

build-system/tasks/helpers.js Outdated Show resolved Hide resolved
return {contents: transformed.code};
}
);

build.onEnd(async (result) => {
Copy link
Member

Choose a reason for hiding this comment

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

FMI: can you explain this refactor to onEnd?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In 8c829df, I changed Babel to output an inline sourcemap for esbuild to ingest, thinking it would remap correctly. But esbuild doesn't carry through names (which is why we switched to the complicated remapping + babelMaps in the first place). I'm correcting it this time, but I'm making the code local to the esbuild-babel plugin instead of spreading it across the plugin and consumer. Now the consumer just gets the correctly remapped sourcemaps.

// We naively prepend content on the first line of during dists and
// serving (for AMP_CONFIG and AMP_EXP). In order for these to not affect
// the sourcemap, we must ensure there is no other content on the first
// line. If you remove this you will annoy Justin. Don't do it.
Copy link
Member

Choose a reason for hiding this comment

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

😢

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was for you. 😜

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants