-
Notifications
You must be signed in to change notification settings - Fork 206
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
Use node package resolution for bundle sources, enable Zoe in cosmic-swingset to use prebundled zcf #1462
Conversation
packages/SwingSet/src/controller.js
Outdated
*/ | ||
function resolveSpecFromConfig(dirname, specPath) { | ||
try { | ||
return require.resolve(specPath); |
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.
If this is given ./vats/vat-timerWrapper.js
, will it return the absolute path of SwingSet/src/vats/vat-timerWrapper.js
instead of something near the config file? That seems surprising. I'm inclined to only call require.resolve
here when specPath
doesn't start with ./
or ../
.
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.
Please investigate using:
function resolveSpecFromConfig(dirname, specPath) {
// This is only resolved by Node's algorithm relative to dirname.
return require.resolve(specPath, { paths: [dirname] });
}
This trick works for all absolute and relative paths, even outside of Node.js packages. No need to be clever when Node.js already does the work for you.
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.
Please see my top-level comment on this PR.
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.
Giving require.resolve
a path eliminates the need to inspect the path to see if it's relative to './'
or '../'
and still takes care of the case that Brian flagged, but by itself it's not sufficient. It will produce a MODULE_NOT_FOUND error for a bare filename (e.g., foo.js
) or a path beginning with a directory name (e.g., subdir/foo.js
), so you still need to do a path.resolve
to handle those cases properly.
48f8a82
to
e979475
Compare
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.
LGTM!
throw e; | ||
} | ||
} | ||
return path.resolve(dirname, specPath); |
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.
Okay, now I see what you're going for. I, personally, would prefer that a specifier is the exact same resolution semantics as for modules (i.e. use ./foo.js
if you mean foo.js
in the current directory). If you agree with that sentiment, you could print a deprecation warning if the path.resolve
is hit.
Up to you, though. I can see the convenience of being able to resolve bare specifiers relative to dirname, even though it gives me the willies. 😄
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.
Would it maybe be better to try path.resolve()
first on bare specifiers, then punt to require.resolve
for everything else or if the resolve doesn't result in a file? That would eliminate the surprise when you do "lodash"
but you meant "./lodash/index.js"
.
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.
Keep in mind that these paths appear inside config files and have a different use pattern from imports. With imports, it's common that a large fraction of the stuff you are referring to comes from someplace else. In config files, non-local references are the exception; you would never reference "./lodash/index.js"
. Not having to stick a redundant ./
in front of almost literally everything improves readability quite a bit.
No description provided.