-
Notifications
You must be signed in to change notification settings - Fork 38
feat: Compatibility with specifier imports #211
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
base: main
Are you sure you want to change the base?
feat: Compatibility with specifier imports #211
Conversation
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.
Looks reasonable with linting fixed.
lib/get-exports.js
Outdated
|
||
// Look for path inside packageJson | ||
let resolvedPath | ||
if (typeof imports == 'object') { |
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.
A better check is Object.prototype.toString.call(imports) === '[object Object]'
.
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.
@jsumners-nr I am surprised by that suggestion (when I just looked at it, I wanted to recommend to check for typeof and not null). That should be much faster and should suffice for the overall check here, if I am not mistaken. Am I missing something / what would the benefit be for calling toString()?
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.
From what I understand, checking with toString() ensures we don't allow values such as [ ], null or Date, which are also of typeof 'object'
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.
I understand that it would not match these anymore. We just check for the property existence on these objects and they should never have the properties we are looking for. Due to that, I favor the faster check.
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.
I agree, I'll add that into the latest changes.
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.
@BridgeAR the original check was a coercive check for an Object
. Various things, e.g Array
, would mistakenly satisfy that check.
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.
@jsumners-nr I reinstated it to the original check after considering that the code only performs operations based on certain properties, which are checked for existence before being used. This allows us to maintain the most efficient check.
21de0c1
to
a153042
Compare
return null | ||
} | ||
} | ||
} catch (error) { |
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.
} catch (error) { | |
} catch { |
Alternatively, we could also just add the cause
895825e
to
d31e359
Compare
3a000d2
to
1f7a010
Compare
1f7a010
to
2022e5e
Compare
This PR adds compatibility with specifiers, for example:
module.exports = require('#main-entry-point');
These specifiers are defined inside the package.json file and can reference either a local file or an external module.
The proposed solution works as follows: whenever a require call begins with # (all specifiers must start with #), locates the package.json, extracts the corresponding reference from the imports field, and sets the newUrl with the resolved path, maintains any original process after that.