-
Notifications
You must be signed in to change notification settings - Fork 17
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
fix(cli): Support multiple library entrypoints #597
Conversation
…e libary entrypoints
…rypoints validators
exports.verifyEntrypoints = ({ | ||
config, | ||
paths, | ||
resolveModule = require.resolve, |
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 was required as it is not currently possible to mock require.resolve
using jest
} | ||
return valid | ||
return !!valid |
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.
The use of bitwise and assignment (&=
) means that valid
will be 1
instead of true
and 0
instead of false
and hence we must coerce valid
to a boolean.
While this could be fixed by using logical and assignment (&&=
), that operator is only supported in node v16+.
Nice 🎉 except for the little edge case mentioned above, this looks good, and nice tests 👍 I merged these changes into the pwa branch to see if it would work for the pwa package with multiple exports, and unfortunately it does not 🤔 I can't tell why not yet |
…port and require exports set
if (expectedExports['.'].import) { | ||
expectedPackage.module = expectedExports['.'].import | ||
} | ||
if (expectedExports['.'].require) { | ||
expectedPackage.main = expectedExports['.'].require | ||
} |
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 is better now that it won't erase module
and main
properties in package.json
, but it also won't validate main
and module
unless exports['.']
directly has .import
and .require
properties (which is not the case if you have:
exports: {
'.': {
browser: {
import: './build/es/index.js',
require: './build/cjs/index.js'
},
worker: { ... }
},
'./etc': { ... }
}
Is it important to have main
and module
properties? If so, you might want to follow down the exports
properties recursively to find import
and require
values; if not, this is fine as it is
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.
Actually I may be misinterpreting this - taking another look
Nvm, confirmed my understanding: in the above case, expectedPackage
does not have expected main
or module
properties
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.
Is it important to have main and module properties?
main
is for node versions 12 and below (which do not support exports
), while module
was a proposal that node does not support so I think it's fine to ignore both when their 'correct' values are ambiguous.
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 cool :)
## [7.2.1](v7.2.0...v7.2.1) (2021-07-27) ### Bug Fixes * **cli:** Support multiple library entrypoints ([#597](#597)) ([a95be81](a95be81))
🎉 This PR is included in version 7.2.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Closes https://jira.dhis2.org/browse/LIBS-202
This PR adds support for multiple library entrypoints defined in d2.config.js, which enables the use of the
exports
field ofpackage.json
for conditional exports. This is particularly useful for libraries which define different entrypoints for web worker/service worker contexts.