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

build(@deephaven/icons): Properly package icons and remove unnecessary files in dist #1437

Merged
merged 5 commits into from
Aug 4, 2023

Conversation

mattrunyon
Copy link
Collaborator

@mattrunyon mattrunyon commented Aug 3, 2023

While updating packages consumed in the docs, I noticed our icons package.json was off. It specified it was a module type (ESM), but exported CJS as .js. This is incorrect as those will be interpreted as ESM by Node. Instead we should be exporting a .cjs file and a .js or .mjs as the main file.

Added an exports section to the package.json to indicate which file is for ESM and which is for CJS. We still need to distribute CJS for Jest tests.

Removed unnecessary files from the distribution. We don't use imports like import vsTrash from '@deephaven/icons/vsTrash'; and the ESM tree shakes, so I removed the individual files exported. They also would have been imported from /icons/dist which is bad practice. Removed the SVG files from the files for the published package as well since they aren't used by anything in production.

BREAKING CHANGE: Any imports/aliasing to @deephaven/icons/dist should be removed and just read the package contents normally (e.g. DHE jest and vite configs for using community packages locally). See the changes to vite and jest configs in this change for how to update

@mattrunyon mattrunyon requested review from dsmmcken and mofojed August 3, 2023 04:59
@mattrunyon mattrunyon self-assigned this Aug 3, 2023
@codecov
Copy link

codecov bot commented Aug 3, 2023

Codecov Report

Merging #1437 (c43d5b2) into main (e3db6bf) will decrease coverage by 0.01%.
Report is 4 commits behind head on main.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1437      +/-   ##
==========================================
- Coverage   45.71%   45.71%   -0.01%     
==========================================
  Files         511      511              
  Lines       35070    35073       +3     
  Branches     8769     8772       +3     
==========================================
+ Hits        16033    16034       +1     
- Misses      18986    18988       +2     
  Partials       51       51              
Flag Coverage Δ
unit 45.71% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 3 files with indirect coverage changes

Copy link
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

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

Publish an alpha and make sure Enterprise still works with the published alpha?

'./packages/icons/dist/index.js'
),
'^@deephaven/(.*)$': path.join(__dirname, './packages/$1/src'),
// All pacakges except icons use src code
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// All pacakges except icons use src code
// All packages except icons use src code

@mattrunyon
Copy link
Collaborator Author

Tested with an alpha and enterprise builds/tests/looks correct.

I put this as a breaking change mostly as a message to update the DHE community packages paths in Vite and Jest (for using the -community scripts). Right now they import from the dist folder directly. I tested these locally w/ the updated configs (matching config in this PR) and they work too

@mattrunyon mattrunyon requested a review from mofojed August 3, 2023 22:40
@mattrunyon mattrunyon merged commit ec7ccef into deephaven:main Aug 4, 2023
@mattrunyon mattrunyon deleted the icon-packaging-fix branch August 4, 2023 20:06
@github-actions github-actions bot locked and limited conversation to collaborators Aug 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants