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

Better interop with NodeJS-native ES Modules #75

Merged
merged 5 commits into from Jun 15, 2021
Merged

Conversation

sjockers
Copy link
Collaborator

@sjockers sjockers commented Jun 9, 2021

Motivation:

Make package work with modern NodeJS (native ES Modules)

Changes:

  • Set type to module so that NodeJS understands that .js files are written in ES Module syntax
  • Rename files that use CommonJS syntax to .cjs to trigger proper handling in NodeJS (specifically used in tests & exports)
  • Add file extensions to imports for better compatibility with ES Modules
  • Provide dual exports so that the package can be both imported and required in NodeJS
  • Explicitly exportessentials and selection builds (ES Modules only — this mimics the previous behavior)
  • Upgrade unit tests (tape & jsdom) to play nice with the new ES Module setup

@1wheel
Copy link
Collaborator

1wheel commented Jun 9, 2021

Awesome! I was just trying to figure out how to do this last night, using v3 of d3-transition as a starting point. Questions:

  • Do we need the sideEffects or peerDependencies properties like d3-transition has? d3-transition also imports a range of packages ("d3-color": "1 - 3",)
  • Since D3 is dropping CommonJS exports does it make sense to bother with duel exports instead of just doing a major version bump?
  • If we're doing a major version bump, should we also switch over to d3v7 once their ESM conversion is finished? I think only a couple of small changes to nestBy, attachTooltip and loadData would be needed along with a refresh of the documentation. cc @gka

@sjockers
Copy link
Collaborator Author

Thanks so much for your feedback!

Do we need the sideEffects or peerDependencies properties like d3-transition has? d3-transition also imports a range of packages ("d3-color": "1 - 3",)

I don’t think it’s absolutely necessary as long as we keep the D3 packages as regular dependencies, but it would probably improve support for tree shaking etc. However, I don't know enough of the specifics of D3 & jetpack to decide what ranges to pick for peerDependencies and what should be a sideEffect. If you have ideas about that, let me know.

Since D3 is dropping CommonJS exports does it make sense to bother with duel exports instead of just doing a major version bump?

Right now I'm mostly trying to get the current version to play nice with ES Modules in order to fix some tooling issues we have Datawrapper. So for now I’d be happy with a new minor version that provides the same exports as before, but in a way that can be properly handled by Node. @gka, what do you think?

@1wheel
Copy link
Collaborator

1wheel commented Jun 10, 2021

D3's API hasn't changed too much since being split into modules and jetpack doesn't hit too many edge cases; I think 1 - 3 should work just fine.

I'm not sure how modern javascript works, but I think peer dependencies of 2-3 for d3-transition and d3-selection might fix #68.

Merging this as is to fix the tooling issue also works; the major version bump can wait till d3's.

@gka gka changed the base branch from master to v2.2.0 June 15, 2021 10:14
@sjockers sjockers merged commit 9326f6b into gka:v2.2.0 Jun 15, 2021
@sjockers
Copy link
Collaborator Author

@1wheel Hey Adam, thanks again for your feedback. I've added peerDependencies and sideEffects per your suggestion. I've also discussed this with @gka and we decided to stick with v1 dependencies for now to keep backwards compatibility. We haven't merged this into master yet because it needs more testing, but we're working on a new release here: #76

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.

None yet

3 participants