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

Prevent node_modules/nan pattern from matching node_modules/nanoid #21396

Merged
merged 1 commit into from Sep 27, 2020

Conversation

smashwilson
Copy link
Member

@smashwilson smashwilson commented Sep 27, 2020

Requirements for Contributing a Bug Fix

Identify the Bug

Likely fix for #21341. We won't really know for sure until CI builds start using a nightly build that includes this fix.

Description of the Change

The function exported from script/lib/include-path-in-packaged-app.js is used to determine whether or not a specific file within the source tree or node_modules is included in the packaged app. It does this by compiling a pair of giant regexps and matching them against path strings. One of these patterns, escapeRegExp(path.join('node_modules', 'nan')), recently began to unexpectedly match one of our transitive dependencies, node_modules/nanoid. (I expect an upgrade to a different package introduced it as a new dependency recently.) This caused the nanoid dependency to be omitted from the application bundle and snapshot, which caused Atom to fail on launch when the dependency was needed (dev and test mode).

Alternate Designs

I'd love to revisit the structure of script/lib/include-path-in-packaged-app.js and find a less brittle mechanism than a giant regexp to include and exclude specific files. Maybe a list of fnmatch globs instead, or some kind of trie structure? Event a giant Set of exact paths might be better.

I also thought about appending a \b to all of the path regexps to prevent the same kind of failures in the future, but ultimately decided to keep this diff short and low-risk.

Possible Drawbacks

None.

Verification Process

I've built Atom from source with script/build --install and verified that the generated artifacts launch correctly. I've also verified that using this Atom build can correctly run a package's test suite.

Release Notes

N/A

@smashwilson smashwilson merged commit 3f919b6 into master Sep 27, 2020
1 check passed
@smashwilson smashwilson deleted the fix-dev-mode branch Sep 27, 2020
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

1 participant