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

Update Shell to use Context.VERSION_ES6 by default #1594

Merged

Conversation

tonygermano
Copy link
Contributor

resolves #1279

@tonygermano
Copy link
Contributor Author

Of course it couldn't have been that easy.

@gbrail
Copy link
Collaborator

gbrail commented Aug 29, 2024 via email

Several tests failed after changing the ShellContextFactory default language version to ES6.
@tonygermano
Copy link
Contributor Author

tonygermano commented Aug 30, 2024

MozillaSuiteTest uses ShellContextFactory (as do some other tests,) but did not explicitly set the version to 1.8, so I'm pushing a change to do that now. Of the handful of failing tests that I looked at, they all used legacy features that are not part of the current javacsript spec (javascript 1.7 generators and let expressions.)

I just did a search, and there are quite a few other tests that reference Context.VERSION_1_8. At some point should we try to update what we can to run with VERSION_ES6 or just leave the old tests at the old language versions and expect newer tests and test262 to cover everything in ES6+?

@tonygermano
Copy link
Contributor Author

I think this one is ready now after my last commit if someone wants to review.

@gbrail
Copy link
Collaborator

gbrail commented Sep 8, 2024

Doing a sanity test myself, but I think that this looks good.

@p-bakker do you think that we can have some way to flag PRs / commits that will need extra attention in the release notes?

And is this a sufficient reason why the next release should be called "1.8.0"?

And should we explore making "VERSION_ES6" the default version for everything?

@gbrail
Copy link
Collaborator

gbrail commented Sep 8, 2024

I think that this is good. I'm going to leave this open for a few days in case any active users want to try it out themselves.

@p-bakker p-bakker added the Potentially Breaking Change Issues that could break backwards compatibility label Sep 9, 2024
@p-bakker
Copy link
Collaborator

p-bakker commented Sep 9, 2024

Tagged the PR with the 'Potential breaking change' label, so we can look for those when making the release notes and include a warning

iMHO this change doesn't warrant a minor version update, as I doubt there's much usage of the shell these days (based on Google search results the shell used to be used to run all kinds of JS tooling that have long since moved to NodeJS or other runtimes). And if there's usage left, I'd think they either aren't likely to upgrade to a newer Rhino version or will use ES6

As for making ES6 the default: personally I rather make a new language version called EcmaScript, which by default doesn't include all non-standard stuff and make that the default, see #1055 (and related #1056, #1057 & #1060)

@tonygermano
Copy link
Contributor Author

@p-bakker are you talking about making the new language version the default elsewhere in the code? The shell has always included the kitchen sink, and I use it quite a bit as a repl for testing both java interop and e4x behavior.

Plain javascript in the rhino shell is not very useful, because it doesn't have any of the APIs that allow it to interact with the outside world without java. Rhino has always been presented as a way to script java using javascript.

@p-bakker
Copy link
Collaborator

p-bakker commented Sep 9, 2024

My thought was to create an EcmaScript language version that just had plain EcmaScript, nothing else and then there would be flags to opt-in on additional things, like Java Interop, E4X, Continuations etc.

Whether or not the Shell would have all those flags turned on our of (by default) would be a decision to be made, but I can see it being useful to have them on by default.

But, this is all just my thoughts, as a group we'd have to come to a consensus how to move forward on things like this 🙂

@tonygermano
Copy link
Contributor Author

I looked at #1055 before I made my last comment, and I like your idea as it applies to embedding, but I do think the shell should at least have java interop turned on by default. Personally, I'd like e4x, too, but if I have to enable that with a command-line option I could live with it (though I also don't see the harm in having it enabled by default in the shell, as it has always been.) And of course there are the shell-only objects that currently get added which would not be part of plain ecmascript.

But until we have all of that implemented, my hope would be that we could move forward with this simple PR with the understanding that it will likely get changed again down the road.

@p-bakker
Copy link
Collaborator

But until we have all of that implemented, my hope would be that we could move forward with this simple PR

Oh, I definitely agree: update to use ES6 now and if we'll get to the other cases at some point, week see them how to proceed with the language version/setup in the shell as well

@gbrail
Copy link
Collaborator

gbrail commented Sep 17, 2024

Yes, this makes sense to me now - thanks!

@gbrail gbrail merged commit b08de52 into mozilla:master Sep 17, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Potentially Breaking Change Issues that could break backwards compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Shell to use Context.VERSION_ES6 by default (instead of Context.VERSION_1_8)
3 participants