bootstrap_node: remove side effects on RegExp #19138
Conversation
TimothyGu
added
the
lib / src
nodejs-github-bot
added
the
process
|
To clarify why I took this approach instead of running the regexes in a new context: Contexts are really memory-expensive, and I'd rather not have to wait for the GC to activate for something we will use only once. It would also complicate the effort to create V8 snapshots for Node.js core (see #17058). |
|
Micro-nit: I'd prefer a more descriptive name for the test and/or a bit more comment explaining what the test is testing for. I'm fine with this landing without these concerns addressed. Just my own opinion/suggestion. Take it or leave it. |
|
this seems like a band-aid, can't we just preprocess the gypi file at build time? |
Yes, but I personally did not want to get into the build system 😄 Feel free to propose a better approach with a new pull request. |
| // convert the Python syntax to proper JSON | ||
| // We cannot use a regex due to side effects on static properties of RegExp | ||
| // (e.g., RegExp.$_). | ||
| let out = ''; |
There was a problem hiding this comment.
Nit: maybe a more descriptive name than "out"?
|
the test will fail on windows for the reasons seen in #19140 (comment) |
|
Closing, as #19140 has been landed. |
|
something to note, 19140 did not fix the fact that the regexp object is still poisoned on windows |
5 participants
Fixes: #18930
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
bootstrap_node