-
Notifications
You must be signed in to change notification settings - Fork 851
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
Conversation
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)); | ||
} | ||
} |
There was a problem hiding this comment.
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.
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 |
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 😛) |
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. |
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 |
That's worth a try -- and TBH I was thinking along the same lines, since we have a few places where we have various "DEBUG" flags that can be set at compile time, and which TBH ought to be configurable at runtime as well. So let me experiment next week and we'll come up with something new. |
I have other ideas so I will close them and propose something. |
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."