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

Add a new context flag for experimental features #1706

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gbrail
Copy link
Collaborator

@gbrail gbrail commented Oct 18, 2024

Add a new flag that we can reuse to guard features that are under development so that we can continue to work on them in the main branch.

Also, add flexibility to the ContextFactory class so that there are other ways to set feature flags other than by creating a subclass.

In practice, this works by adding a capability in ContextFactory to supply a lambda that will determine which features are enabled. I think that this is kind of neat, but looking at the test, I'm not 100% sure it's really all that easier than defining a subclass.

Please take a look at the changes proposed below and let me know if we should proceed with some or all of this.

Regardless, I think that we should use the "FEATURE_EXPERIMENTAL_FEATURES" flag specifically for the Reflect and Proxy implementation, since it works but does not pass enough tests to really be "feature complete."

Add a new flag that we can reuse to guard features that are under
development so that we can continue to work on them in the main branch.

Also, add flexibility to the ContextFactory class so that there are
other ways to set feature flags other than by creating a subclass.
assertFalse(cx.hasFeature(Context.FEATURE_LITTLE_ENDIAN));
assertFalse(cx.hasFeature(Context.FEATURE_DYNAMIC_SCOPE));
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should have a similar test case for every language version and just check if we have to correct feature setup.

@rbri
Copy link
Collaborator

rbri commented Oct 19, 2024

Fine for me, but i'm not sure if we should have such an generic flag, maybe we can follow the original path and have dedicated feature flags for the different features

@p-bakker
Copy link
Collaborator

I think I agree with @rbri: I think I'd want to be able to opt-in to specific experimental features, instead of an all-or-nothing approach

The only 'global' flag that imho would enable multiple experimental features could be a ESNext flag, if we'd ever get into a scenario where we'd have non-finalized EcmaScript proposals (stage 0 through 3) already implemented in Rhino (but I don't think we're quite there yet 😛)

@gbrail
Copy link
Collaborator Author

gbrail commented Oct 23, 2024

What I'm trying to avoid is adding too many feature flags, so we don't end up with "FEATURE_PROXY_REFLECT" and "FEATURE_CLASSES" and so on, because I imagine that over time there will be other big features like this that take a few PRs to implement. But if that doesn't make sense I don't need to do this -- I was hoping that this will be a way to move the work by @rbri on reflect and proxy forward.

@p-bakker
Copy link
Collaborator

What about some other mechanism where you can set which experimental features you want enabled based on strings, so that we can interpret the provided string values and we'd just ignore strings we don't care about (anymore)?

That way we don't have flags in code that we cannot remove without potentially breaking something, we'd just have runtime interpretation of a bunch of strings, where the strings that are actually acted upon varies over time

Think NodeJS uses something similar, with their --harmony-* flags

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