Skip to content
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

🐛 Restore pre 1.2.1 RegExp format, make fully Win compat + less greedy #16

Merged
merged 6 commits into from
Jul 16, 2019

Conversation

depoulo
Copy link
Contributor

@depoulo depoulo commented Jul 8, 2019

Sorry for the confusion 😢

There were a few things going wrong here:

When looking at /node_modules\/(?!(deps|go|here))/ (as of d4d4985), I thought the surrounding slashes were meant as path delimiters (obviously not matching on Windows systems). But I was wrong - those are the RegExp delimiters (the slash in the middle however is a path delimiter).

What that also means is that src/traverse_node_modules/walker.js will match and as such be excluded from transpilation (however, on a Windows machine, src\traverse_node_modules\walker.js won't), just as /node_modules/p-debounce-es5/index.es5.js will be transpiled if the matched deps include p-debounce, as it will match the RegExp.

As of such, my "Windows compatibility" change (9926325, #13) was just plain wrong. If anyone using the according release copies the RegExp and fixes it (by wrapping it in forward slashes or new RegExp('')), it will still work, and be even a bit more specific, in that the above file will no longer match, but projectRoot\node_modules\p-debounce\index.js will now match on a Windows machine. Oh actually it won't since I failed to escape the backslash correctly.

The ultimate solution, however, is to put path delimiters ([\\/] to work in both Windows and POSIX-compatible environments) around node_modules and after the module name (but still inside the negative lookahead). This will remove all theoretical false matches. That is what I'm doing in this pull request, alongside re-adding the surrounding forward slashes to make it a real JavaScript RegExp.

Personally, I'm doing like this:

(package.json)

"postinstall": "sh -c \"yarn --silent are-you-es5 check -r . | tail -n 1 > ./non_ES5_node_modules \"",

(webpack.config.js)

exclude: forLegacyBrowsers
  ? new RegExp(
      fs
        .readFileSync(path.resolve('./non_ES5_node_modules'), 'utf-8')
        // remove surrounding RegExp delimiter slashes and trailing newline
        .slice(1, -2)
    )
  : /[\\/]node_modules[\\/]/,

I had removed it in obahareth#13 (sorry, I had been struck by yarnpkg/yarn#4722)

The behaviour _with_ the trailing path delimiter is more correct, as without it in theory modules starting with the same name as a non-ES5 node_module would be unneccessarily transpiled.

This is all assuming the leading and trailing slash before obahareth#13 were meant as path delimiters and not RegExp delimiters, meaning that anyone wanting to use the RegExp would (and will) have to add surrounding slashes or `new RegExp()` manually, as in `/<place-output-here>/` or `new RegExp('<place-output-here>')` (personally, I'm writing the output of _are-you-es5_ to a file on _postinstall_ and read it into a `new RegExp()` in the webpack config).
@depoulo depoulo changed the title 🐛 Re-add trailing path delimiter to RegExp 🐛 Re-add trailing path delimiter to RegExp Jul 8, 2019
@depoulo depoulo changed the title 🐛 Re-add trailing path delimiter to RegExp Restore pre 1.2.1 RegExp format (still win compat + less greedy) Jul 8, 2019
@depoulo depoulo changed the title Restore pre 1.2.1 RegExp format (still win compat + less greedy) Restore pre 1.2.1 RegExp format, make fully Win compat + less greedy) Jul 8, 2019
@depoulo depoulo changed the title Restore pre 1.2.1 RegExp format, make fully Win compat + less greedy) Restore pre 1.2.1 RegExp format, make fully Win compat + less greedy Jul 8, 2019
@depoulo depoulo changed the title Restore pre 1.2.1 RegExp format, make fully Win compat + less greedy Restore pre 1.2.1 RegExp format (still Win compat + less greedy) Jul 8, 2019
@depoulo depoulo changed the title Restore pre 1.2.1 RegExp format (still Win compat + less greedy) Restore pre 1.2.1 RegExp format, make fully Win compat + less greedy Jul 8, 2019
@depoulo depoulo changed the title Restore pre 1.2.1 RegExp format, make fully Win compat + less greedy 🐛 Restore pre 1.2.1 RegExp format, make fully Win compat + less greedy Jul 8, 2019
@obahareth
Copy link
Owner

Hey @depoulo thanks for the fix, but I see the regex you put is [\\\\/], shouldn't it be [\/\\]?

@depoulo
Copy link
Contributor Author

depoulo commented Jul 8, 2019

Well escaping backslashes is an issue on its own, see the xkcd I linked somewhere. What counts here is the script's CLI output, and that should be correct now.

@depoulo
Copy link
Contributor Author

depoulo commented Jul 8, 2019

(Basically it's one backslash for the Windows path delimiter, another one to escape it for the RegExp, and two more (one for each) since we're inside a string.

@depoulo
Copy link
Contributor Author

depoulo commented Jul 8, 2019

(The forward slash doesn't need to be escaped inside a character group)

@obahareth
Copy link
Owner

Hey @depoulo what I was asking before is because you're matching against \ twice.
Screen Shot 2019-07-13 at 10 23 40 AM

I was suggesting [/\\] instead to match against either a forward or backward slash.
Screen Shot 2019-07-13 at 10 25 34 AM

Does Windows put two consecutive backward slashes?

@depoulo
Copy link
Contributor Author

depoulo commented Jul 13, 2019

Quoting https://xkcd.com/1638/:

Actual backslash, for real this time

see #16 (comment)

Co-Authored-By: Omar Bahareth <omar@omar.engineer>
@obahareth
Copy link
Owner

Thanks again @depoulo, I'll publish a new version later today!

@obahareth obahareth merged commit 51e14db into obahareth:master Jul 16, 2019
@depoulo
Copy link
Contributor Author

depoulo commented Jul 16, 2019

You're welcome, and I have to thank!

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.

None yet

2 participants