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

Drop aliasing global to globalThis in extented compatibility-mode #3864

Open
mstoykov opened this issue Jul 19, 2024 · 0 comments
Open

Drop aliasing global to globalThis in extented compatibility-mode #3864

mstoykov opened this issue Jul 19, 2024 · 0 comments
Labels
breaking change for PRs that need to be mentioned in the breaking changes section of the release notes evaluation needed proposal needs to be validated or tested before fully implementing it in k6

Comments

@mstoykov
Copy link
Contributor

mstoykov commented Jul 19, 2024

What?

As of #3456 the only difference between base and extended compatibility-mode is that extended adds an alias for global that points to globalThis.

This only exists as this was what core-js did and when it was dropped it was readded in #1845.

I don't remember if we had a particular user that complaint about this or not, but it has been a few years and globalThis is pretty much everywhere.

This also has been known to be used as a test if you are running in a browser or NodeJS like environment - which doesn't usually work great with k6.

Solution

Just stop aliasing it - making base and extented the same.

Counter points

The support for this is literally 3 lines

k6/js/bundle.go

Lines 387 to 392 in 111985b

if b.CompatibilityMode == lib.CompatibilityModeExtended {
err = rt.Set("global", rt.GlobalObject())
if err != nil {
return err
}
}

it doesn't really add much

Arguably removing this will require us to explain why extended and base are the same thing 😅

This being used

The only thing I know about that this is being used for is k6chaijs which is due to dependancy.

https://github.com/chaijs/type-detect which is part of chaijs is using it instead of globalThis a patch for this has been done years ago, but no new version has been released in 7 years.

chaijs/type-detect#146 asked for it to be bumped few years ago, and it might be good idea for us to go ahead and start this rolling

I would argue that this is blocking this specifically from going forward, but once this is done we can think about it more.

Related issues

#3860

@mstoykov mstoykov added evaluation needed proposal needs to be validated or tested before fully implementing it in k6 breaking change for PRs that need to be mentioned in the breaking changes section of the release notes labels Jul 19, 2024
mstoykov added a commit that referenced this issue Sep 13, 2024
As part of #3864 it will be nice to
know how much this is used if at all.
mstoykov added a commit that referenced this issue Sep 13, 2024
As part of #3864 it will be nice to
know how much this is used if at all.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change for PRs that need to be mentioned in the breaking changes section of the release notes evaluation needed proposal needs to be validated or tested before fully implementing it in k6
Projects
None yet
Development

No branches or pull requests

1 participant