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

Fix webpack when using browser_wasi_shim as dependency #13

Merged
merged 3 commits into from
Feb 1, 2023
Merged

Conversation

bjorn3
Copy link
Owner

@bjorn3 bjorn3 commented Feb 1, 2023

CommonJS style exports were previously used in index.js despite being an ESM module. Switching to the ESM export syntax fixes webpack.

Fixes #12

CommonJS style exports were previously used in index.js despite being an
ESM module. Switching to the ESM export syntax fixes webpack.

Fixes #12
@bjorn3
Copy link
Owner Author

bjorn3 commented Feb 1, 2023

@brandonchinn178 @bhelx could you please take a look?

@brandonchinn178
Copy link

Looks great! Although you could make it even more concise:

export { WASI } from './wasi.js'
...

Also, I didn't want to make another issue for this, but could you also remove the console.log calls, or at least put them behind a debug flag?

@bhelx
Copy link
Contributor

bhelx commented Feb 1, 2023

Yeah, it definitely should not be mixed. We should go one way or the other. It can be tricky to get something to work in both module formats without a build step though. Typically what I do is use esbuild and provide different entries for CJS and ESM. Will this work for both @brandonchinn178 ?

@brandonchinn178
Copy link

No, it wont work for both, but the question is moot right now because package.json has an exports section that says the package only works with ESM anyway.

A more production-ready deployment would involve a build step outputting both ESM and CJS artifacts and updating package.json to point to the relevant files according to module type. It's the annoying state of the JS ecosystem, but that's what it is.

To sum up:

  • Currently, transpilation is required to import this package with ESM. This package cannot be imported with CJS
  • With this PR, this package can be imported with ESM without any additional transpilation step. This package would still not work with CJS
  • Additional work involving a build step would allow importing this package with both ESM and CJS. But this is less important IMO because most browsers accept ESM, and if you want better compatibility, you're probably using a build step anyway

@bhelx
Copy link
Contributor

bhelx commented Feb 1, 2023

@brandonchinn178 thanks for explaining. I'm okay with merging if it unblocks you. I think I can work around this in my build step.

It's the annoying state of the JS ecosystem, but that's what it is.

I don't know a ton about JS but this is my interpretation as well. There isn't currently a way I've found to make a universal module without some kind of build step. I've found esbuild to be a pretty simple dependency to use that isn't as massive as webpack. I can also work on the build step, but I know @bjorn3 was hoping to avoid that so I'd so I leave it to him to make the final call.

But keep them in case they are ever necessary
@bjorn3
Copy link
Owner Author

bjorn3 commented Feb 1, 2023

Also, I didn't want to make another issue for this, but could you also remove the console.log calls, or at least put them behind a debug flag?

Done

@bjorn3
Copy link
Owner Author

bjorn3 commented Feb 1, 2023

Looks great! Although you could make it even more concise:

Neat.

I can also work on the build step, but I know @bjorn3 was hoping to avoid that so I'd so I leave it to him to make the final call.

I'm fine with having a build step for the module published on npm. I just want to be able to do local development without requiring any build step.

@bhelx
Copy link
Contributor

bhelx commented Feb 1, 2023

Thanks @brandonchinn178 !

I'm fine with having a build step for the module published on npm. I just want to be able to do local development without requiring any build step.

@bjorn3 I'll see if i can figure this out. In my experience once you add a build step it takes over, but I'll test it out.

@bjorn3
Copy link
Owner Author

bjorn3 commented Feb 1, 2023

Should I merge this and publish a new release or should I wait on this build step getting added?

@bhelx
Copy link
Contributor

bhelx commented Feb 1, 2023

I think you're good to merge. I can pin on the previous version or send a follow up if it causes a problem.

@bjorn3 bjorn3 merged commit 597d72f into main Feb 1, 2023
@bjorn3 bjorn3 deleted the esm_fixes branch February 1, 2023 17:00
@bjorn3
Copy link
Owner Author

bjorn3 commented Feb 1, 2023

Published as v0.2.3

@brandonchinn178
Copy link

Local development should be unaffected. The build step should just add more files, not change any of the existing ones, so you should be able to work on it as usual.

Thanks for the quick turnaround!

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.

Don't mix ESM import with CJS exports
3 participants