[breaking] Change the API to choose which features to compile #49

nicolo-ribaudo · 2021-08-13T20:36:26Z

The more we move into the future, the less users will need to compile old regular expression features. However, configuring regexpu-core to only compile some specific features is quite hard. This proposal aims to simplify how the options behave and interact.

Every new syntax feature could be handled in three possible ways:

Compile it to older syntax
Parse it and leave it as-is
Don't parse it (throw an error)

Currently, every regexp feature is supported in a different way:

u is (1) by default, and useUnicodeFlag makes it (2)
s is (1) by default, but regexpu-core respects the s flag only if dotAllFlag is set. useDotAllFlag makes it (2).
\p{...} is (2) by default (if the u flag is enabled), and unicodePropertyEscape makes it (1)
Named groups are (3) by default, and the namedGroup option makes it (1). There are two PRs to allow (2): fix: allow keep named group names when namedGroup is true #39, fix: not throw error when namedGroup is not enabled #41
Lookbehind assertions are (3) by default, and the lookbehind option makes it (2)

All these different options make it hard to configure regexpu-core. I propose a new options system, inspired by how Babel plugins work:

Stable ECMAScript features are parsed by default, and users can opt-in into the transform
ECMAScript proposals are not supported by default, and users can opt-in into parsing or transformation (Note: regexpu-core doesn't support any proposal at the moment)

This means that by default regexpu-core is a no-op (similarly to how Babel does nothing if there are no plugins), however it makes it easier to configure the expected transformation.

Closes #32, closes #37, closes #38, closes #39, closes #41, closes #42

cc @mathiasbynens @JLHwung opinions?

README.md

mathiasbynens · 2021-08-14T06:04:21Z

LGTM. This reminds me to spend some time automating the npm release process. I'll look into that next week.

nicolo-ribaudo · 2021-08-14T22:33:19Z

I'll start working on the implementation to see if there are any problems with this design

I pushed the implementation to match the readme changed

nicolo-ribaudo · 2021-08-15T16:22:35Z

rewrite-pattern.js

-		update(characterClassItem, `(?!${set.toString(regenerateOptions)})[\\s\\S]`)
-	} else {
-		update(characterClassItem, set.toString(regenerateOptions));
+	if (transformed) {


I added this check so that when not transforming the u flag, things like /[\uD806\uDCDF]/u are not modified.

nicolo-ribaudo · 2021-08-15T16:24:11Z

tests/tests.js

@@ -588,14 +600,6 @@ describe('unicodePropertyEscapes', () => {
 			'(?:(?:\\uD838[\\uDEC0-\\uDEF9\\uDEFF]))'
 		);
 	});
-	it('throws without the `u` flag', () => {
-		assert.throws(() => {
-			rewritePattern('\\p{ASCII_Hex_Digit}', '', features);


/\p{ASCII_Hex_Digit}/ is a valid pattern, and it matches the string p{ASCII_Hex_Digit}.

nicolo-ribaudo · 2021-08-15T16:25:58Z

Uh I messed up singular/plural in options names. Do you prefer namedGroup/unicodePropertyEscape, or namedGroups/unicodePropertyEscapes?

jridgewell · 2021-08-15T22:56:43Z

I personally like plural names better, but as long as it's consistent either is fine.

mathiasbynens · 2021-08-16T09:34:55Z

demo.js

-const processedPattern = rewritePattern(pattern, 'u', { useUnicodeFlag: true });
+const processedPattern = rewritePattern(pattern, 'ui', {
+	'unicodeFlag': 'transform'
+})


Suggested change

})

});

Do you want me to open a new PR to setup ESLint? Do you have a preferred config somewhere?

demo.js

rewrite-pattern.js

mathiasbynens · 2021-08-16T09:38:15Z

rewrite-pattern.js

+const validateOptions = (options) => {
+	if (!options) return;
+
+	for (const key of Object.keys(options)) {


How about for (const [key, value] of Object.entries(options))?

This package technically still supports Node.js 4. I wasn't going to propose to update it to Node.js 12 because Babel 7 still supports Node.js 6 too 😅

tests/tests.js

mathiasbynens · 2021-08-16T09:40:05Z

Uh I messed up singular/plural in options names. Do you prefer namedGroup/unicodePropertyEscape, or namedGroups/unicodePropertyEscapes?

Same answer as @jridgewell: plural names sound more natural IMHO but consistency is what matters most.

nicolo-ribaudo · 2021-08-16T10:58:12Z

I prefer plurals too 👍

JLHwung · 2021-08-16T18:32:58Z

README.md

-});
-// → '[\\u{14400}-\\u{14646}]'
-```
+These options can be set to `false`, `'parse'` and `'transform'`. When using `'transform'`, the corresponding features are compiled to older syntax that can run in older browsers. When using `'parse'`, they are parsed and left as-is in the output pattern. When using `false` (the default), they result in a syntax error if used.


It will become a breaking change when an experimental feature becomes stable. For example, let's say we have an imaginative option useSetNotation: boolean | 'parse' | 'transform' for RegExp Set Notation. Now if the proposal advanced to stage 4, regexpu-core will no longer throw for set notation syntax when people are using useSetNotation: false, because stable syntax are always parsed. But they may interpret useSetNotation: false as I want to target to a specific ecmascript version which does not support set notation and thus I disable the syntax. In this case I think it'd better to limit options to only 1) 'parse' and 'transform' or 2) false and transform, and when an experimental option is absent, we should not parse them at all. This approach is exactly how Babel handles features: when an experimental parser plugin is absent we assume users don't want to parse them at all. And when a feature materializes, the parser plugin becomes no-op.

On the other hand, in Babel you can explicitly disable syntax plugins that are enabled somewhere else:

{ "plugins": ["@babel/plugin-syntax-class-static-block"], "env": { "test": { "plugins": [ ["@babel/plugin-syntax-class-static-block", false] ] } } }

When running this with NODE_ENV=test, it will stop throwing in as soon as Babel enables parsing for static blocks by default.

In general, relaxing an error is never considered to be a breaking change. We might change the wording to be clear, but I think that the behavior is fine.

I don't have a strong opinion here. If it helps, I gotta admit that my initial reaction to the current three-value option was one of slight confusion -- I would have expected a simple binary switch (true/false) instead -- but then it made sense when I read the docs in this patch.

mathiasbynens · 2021-12-21T18:48:59Z

My LGTM still stands, so feel free to merge this whenever :) We can always iterate on the details later but let's not block on the options discussion.

This code hasn't been touched in a while, so it's probably good to bring in the newest versions of the dependencies. We can easily tell if there was any incompatible effect on the output. The latest version of filenamify requires using ES modules. We also have to adapt to a breaking change in regexpu-core (see mathiasbynens/regexpu-core#49). Also convert the dependencies to devDependencies, since this tool is not necessary for executing test262.

mathiasbynens previously approved these changes Aug 14, 2021

View reviewed changes

README.md Show resolved Hide resolved

README.md Show resolved Hide resolved

nicolo-ribaudo force-pushed the proposal-new-api branch from 9237950 to a19e8da Compare August 15, 2021 16:18

nicolo-ribaudo force-pushed the proposal-new-api branch from a19e8da to 8e7090c Compare August 15, 2021 16:21

nicolo-ribaudo commented Aug 15, 2021

View reviewed changes

jridgewell approved these changes Aug 15, 2021

View reviewed changes

mathiasbynens approved these changes Aug 16, 2021

View reviewed changes

nicolo-ribaudo force-pushed the proposal-new-api branch 2 times, most recently from 8dd771d to 999bfaf Compare August 16, 2021 12:51

JLHwung reviewed Aug 16, 2021

View reviewed changes

This was referenced Sep 7, 2021

Implement support for && and || in v sets #52

Merged

Support strings in sets in v mode #53

Merged

nicolo-ribaudo added 3 commits December 14, 2021 10:51

[breaking] Write readme for the new API interface

61ba424

Implementation

eb21ab4

Review

d884d28

nicolo-ribaudo force-pushed the proposal-new-api branch from 999bfaf to d884d28 Compare December 14, 2021 09:51

nicolo-ribaudo changed the title ~~[breaking][proposal] Change the API to choose which features to compile~~ [breaking] Change the API to choose which features to compile Dec 21, 2021

nicolo-ribaudo merged commit c10651d into mathiasbynens:main Dec 21, 2021

nicolo-ribaudo deleted the proposal-new-api branch December 21, 2021 18:53

mathiasbynens mentioned this pull request Dec 22, 2021

Support RegExp set notation + properties of strings proposal #51

Closed

9 tasks

mathiasbynens mentioned this pull request Jan 10, 2022

Update regexpu’s API following the breaking regexpu-core changes in #49 #55

Open

mathiasbynens mentioned this pull request Apr 25, 2022

Update regexpu’s API following the breaking regexpu-core changes mathiasbynens/regexpu#76

Open

[breaking] Change the API to choose which features to compile #49

[breaking] Change the API to choose which features to compile #49

Uh oh!

Conversation

nicolo-ribaudo commented Aug 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mathiasbynens commented Aug 14, 2021

Uh oh!

nicolo-ribaudo commented Aug 14, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolo-ribaudo commented Aug 15, 2021

Uh oh!

jridgewell commented Aug 15, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mathiasbynens commented Aug 16, 2021

Uh oh!

nicolo-ribaudo commented Aug 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mathiasbynens commented Dec 21, 2021

Uh oh!

Uh oh!

nicolo-ribaudo commented Aug 13, 2021 •

edited

Loading