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

fix(cli): Support multiple library entrypoints #597

Merged
merged 9 commits into from
Jul 27, 2021
Merged

Conversation

mediremi
Copy link
Contributor

@mediremi mediremi commented Jul 26, 2021

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 of package.json for conditional exports. This is particularly useful for libraries which define different entrypoints for web worker/service worker contexts.

exports.verifyEntrypoints = ({
config,
paths,
resolveModule = require.resolve,
Copy link
Contributor Author

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
Copy link
Contributor Author

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+.

@KaiVandivier
Copy link
Contributor

KaiVandivier commented Jul 26, 2021

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

Comment on lines +118 to +123
if (expectedExports['.'].import) {
expectedPackage.module = expectedExports['.'].import
}
if (expectedExports['.'].require) {
expectedPackage.main = expectedExports['.'].require
}
Copy link
Contributor

@KaiVandivier KaiVandivier Jul 26, 2021

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

Copy link
Contributor

@KaiVandivier KaiVandivier Jul 26, 2021

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

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay cool :)

@mediremi mediremi merged commit a95be81 into master Jul 27, 2021
@mediremi mediremi deleted the fix/LIBS-202 branch July 27, 2021 09:28
dhis2-bot added a commit that referenced this pull request Jul 27, 2021
## [7.2.1](v7.2.0...v7.2.1) (2021-07-27)

### Bug Fixes

* **cli:** Support multiple library entrypoints ([#597](#597)) ([a95be81](a95be81))
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 7.2.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants