regex source property escaping value changed?

[krohrsb]

• Version: 12.10.0 -> 12.11.0
• Platform: Darwin mtvl1502b5c63 18.7.0 Darwin Kernel Version 18.7.0: Thu Jun 20 18:42:21 PDT 2019; root:xnu-4903.270.47~4/RELEASE_X86_64 x86_64
• Subsystem: ?

It would appear the regex source property behavior changed between node versions 12.10.0 and 12.11.0

> process.versions.node
'12.10.0'
> /[\\/]node_modules[\\/]/.source
'[\\\\\\/]node_modules[\\\\\\/]'

> process.versions.node
'12.11.0'
> /[\\/]node_modules[\\/]/.source
'[\\\\/]node_modules[\\\\/]'

It would appear the generated string with escaped slashes differs.

Granted, the regex itself behaves the same (as far as I can tell) any tooling/logic depending on the value of source may be impacted (for example Jest snapshots).

If this was intended behavior and listed in a changelog, I could not locate it.

[addaleax]

If this was intended behavior and listed in a changelog, I could not locate it.

I think it is expected behavior – it matches my understanding of the JS spec – but it’s not in our changelog because this came along with a JS engine upgrade. Unfortunately, V8 itself does not always keep detailed changelogs.

[Hakerh400]

Bisected to v8/v8@17b9d87

[krohrsb]

Thanks for the reference! Knowing this I can take the appropriate actions for my use-case safely. Unsure if there is anything else you'd like to do with the issue, but feel free to close if necessary.

[saifali96]

I just spent the last 4 hours debugging our React Native application release build script. It is failing since today, I wouldn't have realized it was a node/v8 issue. I went through the changelog in the morning to make sure there are no clashes.

error Invalid regular expression: /(.*\\__fixtures__\\.*|node_modules[\\\]react[\\\]dist[\\\].*|website\\node_modules\\.*|heapCapture\\bundle\.js|.*\\__tests__\\.*)$/: 
Unterminated character class. 

Run CLI with --verbose flag for more details.
FAILURE: Build failed with an exception. 

Shall we expect a fix from node or from the library maintainers?

[addaleax]

Shall we expect a fix from node or from the library maintainers?

I guess we could consider reverting this for Node 12 if there’s agreement that we should do that, but given that it’s a bugfix (it aligns us with the spec and other browsers) and coming from V8 rather than us directly, I wouldn’t be a huge fan of that.

/cc @nodejs/v8 @nodejs/tsc

[cjihrig]

I would prefer we not revert and let the issue be handled (or not) upstream.

[addaleax]

@cjihrig I assume V8 would not be reverting anything here because from their perspective V8 7.7 was already a major release and they can do breaking changes in it.

[cjihrig]

Yea, that's what I assumed.

[saifali96]

With a little more digging, I found this PR facebook/metro#454 for at least the library used by React Native, to make it compliant with the new V8 regex tests, but I wonder what else is broken..

[schuay]

Just to confirm, v8/v8@17b9d87 was an intentional change to better align V8 with the spec.

I'm not sure I fully understand if there is a bug. Are you saying that V8's current behavior is incorrect?

[targos]

I'm going to close this issue.

It's unfortunate that the bug fix affects some users, but this can always be the case.
The error reported by @saifali96 is not related to this. The same regular expression is also considered invalid in Node.js 10 and earlier.