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

Strip hygiene code in favor of UUIDs #83

Merged
merged 2 commits into from
Nov 5, 2024

Conversation

wagenet
Copy link
Contributor

@wagenet wagenet commented Oct 31, 2024

Fixes #71

After conversation with @wycats and reading @chancancode's comments in #71, it seems that our only concern here is making sure that the template import doesn't conflict. If we don't care about potentially importing multiple times (we can't see why we need to), then we can avoid the whole issue by always just importing it with a UUID.

@@ -94,7 +98,7 @@ describe(`process`, function () {
let output = p.process(`<template>Hi</template>`, { inline_source_map: true });

expect(output).to.match(
/sourceMappingURL=data:application\/json;base64,eyJ2ZXJzaW9uIjozLCJzb3VyY2VzIjpbIjxhbm9uPiJdLCJzb3VyY2VzQ29udGVudCI6WyI8dGVtcGxhdGU-SGk8L3RlbXBsYXRlPiJdLCJuYW1lcyI6W10sIm1hcHBpbmdzIjoiO0FBQUEsZUFBQSxTQUFVLENBQUEsRUFBRSxDQUFBLEVBQUE7SUFBQTtRQUFBLE9BQUEsS0FBQSxTQUFBLENBQUEsRUFBVztJQUFEO0FBQUEsR0FBQyJ9/
/sourceMappingURL=data:application\/json;base64,/
Copy link
Contributor

Choose a reason for hiding this comment

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

is this right? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I know nothing about source maps.

and... currently our gjs/gts sourcemaps are very weird in-browser

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's pretty hard to check the exact source map now that the template imports gets a UUID. This at least confirms that we did get an inline source map of some sort.

Copy link
Contributor

Choose a reason for hiding this comment

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

We may need to use magic-string or something (whatever the SWC equiv is) to keep a correct sourcemap. I've just figured out how to get gjs/gts sourcemaps in the browser over here: embroider-build/embroider#2162

Choose a reason for hiding this comment

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

This looks like an empty source map though

Copy link
Contributor

Choose a reason for hiding this comment

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

what I missed is that this is just a regex matcher -- it's less precise, but ensures that the sourcemap is present, but doesn't check content

Choose a reason for hiding this comment

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

Ah, got it

@ef4 ef4 added the bug Something isn't working label Nov 5, 2024
@ef4 ef4 merged commit 578867f into embroider-build:main Nov 5, 2024
1 check passed
@github-actions github-actions bot mentioned this pull request Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bad transform of fake this param
4 participants