-
-
Notifications
You must be signed in to change notification settings - Fork 128
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
Improve loading of submodules #1940
base: master
Are you sure you want to change the base?
Conversation
lib[subName] = sub; | ||
try { | ||
const sub = appRequire(name + '/' + subName); | ||
if (lib[subName] && lib[subName] === sub[subName]) continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate expression: lib[subName]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check is actually necessary here because in every other case (except the one I described above) lib[subName]
and sub[subName]
would be equal to undefined
.
lib/deps.js
Outdated
if (lib[subName] && lib[subName] === sub[subName]) continue; | ||
lib[subName] = sub; | ||
} catch (e) { | ||
if (e.message.startsWith("Cannot find module '")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- We do not use double quotas
"
, use linter before commit - Parsing error in a such way is not reliable, it's better to get text between two double quotas with
metautil.between(e.message, '"', '"')
see: https://github.com/metarhia/metautil/tree/master#strings-utilities e
is a bad naming, bettererr
or evenerror
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lib/deps.js
Outdated
lib[subName] = sub; | ||
} catch (e) { | ||
if (e.message.startsWith("Cannot find module '")) { | ||
const moduleName = e.message.substring(20, e.message.indexOf("'\n")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
20 is a magic number
lib/deps.js
Outdated
} catch (e) { | ||
if (e.message.startsWith("Cannot find module '")) { | ||
const moduleName = e.message.substring(20, e.message.indexOf("'\n")); | ||
const optional = pkg.peerDependenciesMeta?.[moduleName]?.optional; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double optional chaining is not ok in out codebase
lib/deps.js
Outdated
if (optional) continue; | ||
else throw e; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just remove else
here
1. Don't load submodules with not installed optional dependencies
Some packages may have optional dependencies.
If there was an error during loading a submodue, and it's a missing optional dependency error, just skip this submodule.
Closes #1937
2. Skip submodules if they have already been loaded as a part of main module
Example:
some-module
package.json
index.js
submodule.js
The main module exports all from the submodule.
And the submodule exports a function with the same name as a submodule name.
So when loading the submodule,
lib
already have a propertysubmodule
.Closes #1938
npm t
)npm run fmt
)