Skip to content

show bug with implementation of lte (et al) as keyword#21373

Merged
kategengler merged 3 commits intomainfrom
js-scope-polution
May 6, 2026
Merged

show bug with implementation of lte (et al) as keyword#21373
kategengler merged 3 commits intomainfrom
js-scope-polution

Conversation

@mansona
Copy link
Copy Markdown
Member

@mansona mansona commented May 5, 2026

I discussed this with @NullVoxPopuli last week, but the way that we have implemented all the "X as keyword" PRs (where they automatically add an import) means that the named import now pollutes the JS namespace for the whole file.

The example is a bit contrived in this PR but it demos the problem e.g. we expect this smoke test to fail but it doesn't 🙈

Edit: thanks to @kategengler for the hint on how to make this fail when we have the bad behaviour 🎉

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 5, 2026

📊 Size report

Tarball size1.2 MB1.2 MB

dist/dev   0%↑

File Before (Size / Brotli) After (Size / Brotli)
Total (Includes all files) 2.1 MB / 477.2 kB 0%↑2.1 MB / 0%↑477.3 kB

dist/prod   0%↑

File Before (Size / Brotli) After (Size / Brotli)
Total (Includes all files) 1.9 MB / 436.8 kB 0%↑1.9 MB / 0.01%↑436.8 kB

smoke-tests/v2-app-hello-world-template/dist   No changes

File Before (Size / Brotli) After (Size / Brotli)
Total (Includes all files) 251.6 kB / 68.8 kB 251.6 kB / 68.8 kB

🤖 This report was automatically generated by wyvox/pkg-size

@NullVoxPopuli NullVoxPopuli marked this pull request as ready for review May 6, 2026 04:24
) {
if (env.meta?.jsutils) {
node.original = env.meta.jsutils.bindImport(moduleSpecifier, name, node, {
name,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

turns out "name" isn't a thing

@mansona mansona requested a review from a team May 6, 2026 12:06
@mansona
Copy link
Copy Markdown
Member Author

mansona commented May 6, 2026

I requested a review from someone else in framework since I made the test and you did the fix @NullVoxPopuli 😂

@NullVoxPopuli
Copy link
Copy Markdown
Contributor

cahoots!

Copy link
Copy Markdown
Member

@kategengler kategengler left a comment

Choose a reason for hiding this comment

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

Should we have a more general test for this for all keywords too?

@NullVoxPopuli
Copy link
Copy Markdown
Contributor

they all use the same codepath (what was changed), so testing more keywords would be redundant

@kategengler kategengler merged commit 5d773bc into main May 6, 2026
38 checks passed
@kategengler kategengler deleted the js-scope-polution branch May 6, 2026 15:27
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 this pull request may close these issues.

3 participants