Skip to content

Conversation

Maxim-Mazurok
Copy link
Contributor

@DmitrySoshnikov
Copy link
Owner

@Maxim-Mazurok thanks for the PR. Can we confirm all the tests pass, and do we need to handle other symbols?

@Maxim-Mazurok
Copy link
Contributor Author

Can we confirm all the tests pass

Confirming that all tests are passing:
image

do we need to handle other symbols?

I couldn't really think of anything that would have similar characteristics...

For example /(\d|[\n\r])/gm gets optimized into /([\d\n\r])/gm which is correct. Basically, we only need to care about symbols that mean different things depending on [] context.

ChatGPT suggested to check ^, - and \ which all seem to work fine. For example, - works fine because of this if I understand correctly:

/**
* @param {Object} expression - Char or ClassRange node
* @returns {number}
*/
function getSortValue(expression) {
if (expression.type === 'Char') {
if (expression.value === '-') {
return Infinity;
}
if (expression.kind === 'control') {
return Infinity;
}

So no, I don't think we need to handle other symbols.

@DmitrySoshnikov
Copy link
Owner

Looks good, thanks for the contribution!

@DmitrySoshnikov DmitrySoshnikov merged commit 312de0b into DmitrySoshnikov:master Apr 16, 2023
@Maxim-Mazurok
Copy link
Contributor Author

Hi @DmitrySoshnikov , looks like this PR was supposed to be included in v0.1.25 however I can't find expected "meta" word in https://www.unpkg.com/[email protected]/dist/optimizer/transforms/group-single-chars-to-char-class.js and the issue is still reproducible in v0.1.25.

Perhaps it needs to be rebuilt and republished? Thank you!

@DmitrySoshnikov
Copy link
Owner

@Maxim-Mazurok thanks for the heads up, yes, seems the build script is broken and didn't recompile the code (in fact, npm run build doesn't work anymore, I'll take a look).

@DmitrySoshnikov
Copy link
Owner

@Maxim-Mazurok should be fixed in v0.1.27. Please let me know if you see any issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

optimizer incorrectly proposes rewrite .|[\r] as [.\r] better-regex proposes to replace (.|[\r\n]) with [.\r\n]
2 participants