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

Switch to esm #13

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Switch to esm #13

wants to merge 4 commits into from

Conversation

jimmywarting
Copy link

@jimmywarting jimmywarting commented Apr 21, 2023

Couple of things:

  • Executed standard * --fix to format the code to look the same. it almost looked like you followed that pattern.
  • Switched code to ESM import / export ( allows for top level await and other goodies )
  • Updated the test to require node v18 that have fetch + atob built in globally
  • removed the window. prefix from window.crypto as NodeJS have a spec'ed web crypto on the global scope now.
    • besides, you should now use globalThis instead of window, self or this... it's the new standard that exist in all env.
  • removed all dev-dep
  • updated workflow to test out v18 instead.

@JonahGroendal
Copy link
Owner

I appreciate this and I'll take a look at it. A couple issues stand out from the description tho:

  1. This is designed to work in a browser rather than nodejs, so window is required (unless globalThis works in the browser in place of window?).

  2. I'm a little hesitant to switch to ESM in case it breaks projects downstream. Plus, what use do we have for top-level await?

@JonahGroendal
Copy link
Owner

ok looks like globalThis does work in the browser and ESM probably wont break anything

@jimmywarting
Copy link
Author

Top level await might not bring any benefits right now...
but using ESM allows the browser to more easily import other dependencies that dose not have a package mananger like npm.

in browser you can either use async import from anyware

<script>
  import('acme-easy').then(module => { ... })
</script>
But you can also do:
<script type="module">
  import client from 'acme-easy'
</script>

With import syntax you can also specify a import map to basically change cdn and how it should resolve dependencies.

Another benefit would also be that this would then also work in Deno if you switched to ESM. (btw Deno don't have support for cjs - it only target ESM)

@JonahGroendal
Copy link
Owner

I'll merge this right after I fix an issue with the protocol implementation

@jimmywarting
Copy link
Author

jimmywarting commented Apr 26, 2023

Would you like me to divide this up to separate PRs?

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.

2 participants