-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Complete nodejs data for RegExp #6618
Conversation
Hey @gilmoreorless, thanks for tackling this! Regarding the Node.js browser data, I'm thinking that it may make sense to change 0.9.1 to 0.8.0, since they're both the same V8 version (confirmed on https://nodejs.org/en/download/releases/). It may be better to follow it up in another PR -- I'm going to do some more digging if there were some notable changes between the two versions. |
I agree with consolidating versions, but I had a memory that 0.9.1 was added specifically for a non-V8-related feature. I also have an annoyance with 0.1.100 being the first version listed in the BCD files. I believe it's because |
This is a fair point, @gilmoreorless. I've only though about it for a minute here, but it seems like permitting |
Hi @gilmoreorless, I'm sorry this seems to have fallen through the cracks. Do you know if any of the data here is affected by the distinction between |
@ddbeck Don't worry, it slipped off my radar too! I've raised #6861 to cover the discussion about the minimum version. For this PR, all the |
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.
Thank you for your incredible patience on this, @gilmoreorless. I have one suggestion on a comment. Otherwise, this looks great.
(Expect an answer on the Node.js version question soon!)
@@ -1765,7 +1765,8 @@ | |||
"version_added": false | |||
}, | |||
"nodejs": { | |||
"version_added": true | |||
"version_added": "6.0.0", | |||
"notes": "Case folding is implemented in version 8.6.0" |
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.
Can we reframe this a little, to front-load the fact that it's a historic deficiency?
"notes": "Case folding is implemented in version 8.6.0" | |
"notes": "Before 8.6.0, case folding is not supported." |
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.
Good suggestion. I think I chose that wording to be consistent with the existing note for edge
a few lines above. I'm happy to change it, but probably both notes should be changed together for consistency.
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.
Ah, I missed that. Yeah, let's do that in a separate PR. I'll approve and merge this now. Thank you! 🎉
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.
🚀
Part of #6249. This fills in all the
true
andnull
values for Node inRegExp
.Most of the entries were supported in the first version of Node, because they were also in Chrome v1. For the ones that remained I checked the data against Chrome/V8 releases and https://node.green, as well as some manual testing.
There was one oddity I noticed in the browser data though, for
javascript.builtins.RegExp.source.empty_regex_string
. This was only added to V8 in version 3.10.6 (release notes), which means Node 0.8.0 (backed up by manual testing). This equates to Chrome 20 (according to Wikipedia, anyway), but the data currently lists Chrome 6 for that entry. I've left the Chrome entry alone here, but it might be worth a separate PR.