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

Added ESM version #49

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

niklaswolf
Copy link

See #47
I just added a copy of index.js and changed it to esm. I don't like the code duplication, but I didn't want to introduce a transpiler to output different versions as that would be a big change. If you feel comfortable with doing so, I'd be happy to remove the code duplication.

@iansinnott
Copy link
Owner

Yeah, the duplication makes me uncomfortable as well because it's the entire file. If we want to change anything at all we'll have to change it in two places and someone might forget, causing inconsistency/bugs.

What are your thoughts on removing the duplication?

We could definitely add a build step and then use use this ESM version as the source fo truth. I like the simplicity of just having JS, but I'm open to bringing in webpack.

@niklaswolf
Copy link
Author

niklaswolf commented Feb 3, 2020

I'd love to remove the code duplication, I just did not want to introduce such a big change (add build step) without having your opinion on that. But as you're fine with it, I'm happy to update the PR with a build step using the ES version as source. I don't think we need Webpack at all, since there is nothing to be bundled. Using Babel to transpile the ES version to CommonJS should be sufficiant.

EDIT: damn, forgot about the imports, so bundler is needed, you're right

@niklaswolf
Copy link
Author

What do you think about the export format? I don't see any value in making a commonJS export, as this will never be run on node alone. A export for IIFE (browsers) seems more suitable. Besides that, every bundled React application will use the ES version I think, so the only value for having another export format is when people import this directly via a script tag.
What do you think?

@iansinnott
Copy link
Owner

Maybe you're right about just using babel. This is meant for react apps so whoever is importing this lib will probably have some kind of build pipeline set up. If we can transpile ES code to the form it is now (CommonJS) then that should allow moving forward without breaking anyone's build, right?

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

2 participants