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

Exporting as ESM and CJS #181

Closed
wants to merge 2 commits into from

Conversation

anthonyalayo
Copy link
Contributor

  1. don't bundle modules, that allows for more effective tree shaking
  2. remove the browser export, it seems unnecessary in 2023
  3. keep exporting cjs as a single file
  4. move emotion into peer dependencies, which is recommended by the project

This presently does not build, but it's almost there. I'll need a bit of help @pionxzh @rtritto

@netlify
Copy link

netlify bot commented Jan 17, 2023

Deploy Preview for any-viewer ready!

Name Link
🔨 Latest commit bf5a86e
🔍 Latest deploy log https://app.netlify.com/sites/any-viewer/deploys/64169e3669a81b00088b59fc
😎 Deploy Preview https://deploy-preview-181--any-viewer.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@rtritto
Copy link
Contributor

rtritto commented Jan 17, 2023

Should we convert this project to ESM insead of convert it for export?
ESM Guide: Pure ESM package

  1. move emotion into peer dependencies, which is recommended by the project

I think it's better to split this in a separated PR

@pionxzh
Copy link
Collaborator

pionxzh commented Jan 17, 2023

Should we convert this project to ESM insead of convert it for export? ESM Guide: Pure ESM package

  1. move emotion into peer dependencies, which is recommended by the project

I think it's better to split this in a separated PR

yes, let's move it to another one.

@pionxzh
Copy link
Collaborator

pionxzh commented Jan 17, 2023

Let me check what's wrong. 👌 Sorry I was so busy.

@@ -115,7 +109,7 @@ const dtsMatrix = (): RollupOptions[] => {
cache,
output: {
file: resolve(outputDir, `${output}.d.ts`),
format: 'es'
format: 'umd'
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we just use cjs instead?

@pionxzh
Copy link
Collaborator

pionxzh commented Jan 18, 2023

About 2, I need to think and ask more about why they were provided at first.

@pionxzh
Copy link
Collaborator

pionxzh commented Jan 19, 2023

Can you move these 2 to another PR so we can merge that first?

  • Delete tsconfig.node.json
  • Remove @types/web

And this can be another

"sideEffects": false

So this PR will focus on the changes of the bundling process and dropping the browser file (which I still can't decide yet.)

@rtritto
Copy link
Contributor

rtritto commented Jan 19, 2023

Needed these changes #181 (comment) too

@pionxzh
Copy link
Collaborator

pionxzh commented Jan 19, 2023

Needed these changes #181 (comment) too

Not sure if it is just me or what. I couldn't see the content of your comment 😵‍💫

@rtritto
Copy link
Contributor

rtritto commented Jan 19, 2023

Just check the review comments

@pionxzh
Copy link
Collaborator

pionxzh commented Mar 19, 2023

@anthonyalayo Sorry that we did a rebasing to remove some sensitive content from the history. I just rebased this PR to fix the history.

I think we can close this first as most of the content has been split to other PRs. And we don't have a plan to drop the browser build. Is that ok?

@codecov
Copy link

codecov bot commented Mar 19, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01 🎉

Comparison is base (f5411a2) 87.63% compared to head (bf5a86e) 87.65%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #181      +/-   ##
==========================================
+ Coverage   87.63%   87.65%   +0.01%     
==========================================
  Files          18       18              
  Lines        1982     1984       +2     
  Branches      358      358              
==========================================
+ Hits         1737     1739       +2     
  Misses        245      245              
Impacted Files Coverage Δ
src/stores/JsonViewerStore.ts 92.24% <100.00%> (+0.06%) ⬆️
src/stores/typeRegistry.tsx 94.75% <100.00%> (+0.01%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@anthonyalayo
Copy link
Contributor Author

Sounds good!

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.

3 participants