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

[colors] fix: output lib/cjs and lib/esm modules #6179

Merged
merged 1 commit into from
May 24, 2023

Conversation

adidahiya
Copy link
Contributor

Changes proposed in this pull request:

@blueprintjs/colors was set up with a nonstandard /lib folder from the beginning. This PR updates the build outputs to match the other @blueprintjs packages.

Submodule paths are not part of our public API (users shouldn't be reaching into /lib/ manually), so it's not a breaking change.

@adidahiya adidahiya mentioned this pull request May 24, 2023
2 tasks
@adidahiya
Copy link
Contributor Author

[colors] fix: output lib/cjs and lib/esm modules

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@adidahiya adidahiya merged commit 3156db5 into develop May 24, 2023
@adidahiya adidahiya deleted the ad/fix-colors-lib branch May 24, 2023 17:47
@shim-flounce
Copy link
Contributor

Hm for some reason this now results in tslib being pulled in where previously this was just Object.assign. Check line 16 of https://unpkg.com/browse/@blueprintjs/colors@4.2.0/lib/esm/colors.js and 131 of https://unpkg.com/browse/@blueprintjs/colors@4.1.24/lib/colors.js

With strict package managers such as Yarn in PnP mode this won’t work because the colors package doesn’t list tslib as a dependency.

"lib": ["es6", "dom"],
"module": "commonjs",
"outDir": "../lib",
"target": "ES2015"
Copy link
Contributor

@shim-flounce shim-flounce May 25, 2023

Choose a reason for hiding this comment

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

I believe this is caused by this. Instead of targeting ES2015, it now targets ES5.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add in the tslib dependency. It's better for the packages to be consistent. We can consider migrating to "target": "es2015" in a future major version.

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

Successfully merging this pull request may close these issues.

2 participants