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

Remove installable 'natives' feature that modifies String and RegExp prototypes #89

Closed
retorquere opened this issue Jun 28, 2015 · 10 comments · Fixed by #207
Closed

Remove installable 'natives' feature that modifies String and RegExp prototypes #89

retorquere opened this issue Jun 28, 2015 · 10 comments · Fixed by #207
Labels
Milestone

Comments

@retorquere
Copy link

I'm including xregexp in a firefox extension, and the changes to RegExp.prototype and String.prototype don't pass the impending extension signature validation. Is it possible to get an xregexp build that doesn't make these modifications to these prototypes?

@slevithan
Copy link
Owner

Hm, interesting. There's very little benefit to adding this option, since it would save almost no code, the "natives" option is already self-contained and not relied on by the rest of the code, and native overrides are already off by default. You'd essentially just need to comment out xregexp.js#L376-L380 or kill the setNatives function and its calls in install and uninstall. I'd recommend doing that for now, but will leave this open for more feedback.

@retorquere
Copy link
Author

retorquere commented Jun 29, 2015 via email

@slevithan
Copy link
Owner

Would be happy to see a pull request that showed how a custom build that worked around this problem would work and be set up.

I'm also considering removing support for XRegExp.install('natives') in the next major breaking change (XRegExp 4.0.0), which would resolve this since it would remove the lines in question for everyone.

@retorquere
Copy link
Author

That's in essence what my pull request would do.

@slevithan slevithan added this to the v4.0.0 milestone Oct 10, 2015
@wilmoore
Copy link

I'm also considering removing support for XRegExp.install('natives') in the next major breaking change (XRegExp 4.0.0), which would resolve this since it would remove the lines in question for everyone.

Before reading the above I was going to suggest something similar. Move XRegExp.install('natives') into a separate package that can optionally be installed by anyone who cared to.

@slevithan
Copy link
Owner

XRegExp.install('natives') was marked as deprecated in commit 57b3172.

@retorquere
Copy link
Author

Super. Any timeline on a release which would contain this change?

@slevithan
Copy link
Owner

slevithan commented Feb 24, 2016

This will be included in v4.0.0. Sorry, no timeline at the moment.

There's no required delay before releasing v4.0.0, but I would like to bundle the drop of support for XRegExp.install('natives') with at least one other planned breaking change: #108 (drop all support for pre-ES5 browsers). These changes might take a while since I'm not able to dedicate much time to the project at the moment.

@slevithan slevithan changed the title Is it possible to get a build that does not add to the native String and Regexp prototypes? Remove XRegExp('natives') and no longer add to String and Regexp prototypes Apr 17, 2017
@slevithan slevithan changed the title Remove XRegExp('natives') and no longer add to String and Regexp prototypes Remove installable 'natives' feature that modifies String and RegExp prototypes Apr 17, 2017
@slevithan
Copy link
Owner

After making this change, will also want to update tests/helpers/h.js to no longer try to uninstall 'natives'.

@slevithan
Copy link
Owner

@retorquere this is finally completed and included in v4.0.0, thanks to @josephfrazier.

speecyy added a commit to speecyy/xregexp that referenced this issue Jan 6, 2018
3590212051 added a commit to 3590212051/POPmotion that referenced this issue Jan 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants