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

This PR provides a simple wrapper to provide correct ESM export #10

Closed
wants to merge 1 commit into from
Closed

This PR provides a simple wrapper to provide correct ESM export #10

wants to merge 1 commit into from

Conversation

typhonrt
Copy link

So, given the complexity of five-server and that there really is only one export the best solution I could come up with is providing an index.mjs wrapper which simply provides a correct ESM default export from the CJS code.

I added the proper entries into package.json as well. It certainly will work Node 12.6+ when the exports field was introduced for package.json. It should work prior as well w/ "module" added for below Node 12.6 though a test or two couldn't hurt to check.

@typhonrt typhonrt mentioned this pull request May 12, 2021
@yandeu
Copy link
Owner

yandeu commented May 16, 2021

Good ideas to just add a ESM wrapper!

But it did not work as expected :/

I wrote an alternative that seems to work 7198e5a.

@typhonrt
Copy link
Author

@yandeu Yeah.. Not exactly sure what didn't work for you re: the this PR as I did test it, but I'm just glad a solution can get added to five-server. Can you publish a 0.0.25 release with it soon?

@typhonrt typhonrt closed this May 19, 2021
@yandeu
Copy link
Owner

yandeu commented May 20, 2021

@typhonrt I'm sorry, but it just has too many issues. I don't see a way to make it work without "exports", which limits the possibilities a lot :/

Node.js, ESM, etc. is just not ready in my opinion. On the other hand, TypeScript works great since the beginning. I just added a very simple ESM wrapper and added import documentation to the README file.

Hope this helps :)

@typhonrt
Copy link
Author

No worries.. if you look at the files changed in the PR "exports" is used. As long what you have gets into the next update all is good.

if you are referencing "module": "lib-esm/index.mjs" that is a fallback for Node 12.0 - 12.6 before exports was introduced.

@yandeu
Copy link
Owner

yandeu commented May 20, 2021

I believe the issue is the "exports". It only allows me to access the files ./lib-esm/index.mjs, ./lib/index.js, ./package.json. But I need access to all files inside ./lib/ from all types of modules.

In the package five-server-vscode, for example, I access five-server/lib/misc, and five-server/lib/msg which is blocked.

I have just published v26

@typhonrt
Copy link
Author

Ah yes... See: subpath patterns & subpath folder mappings

Granted subpath patterns was introduced in Node 12.20 / 14.13 w/ subpath folder mappings being the older way to handle things. This particular issue isn't ESM related.. Awesome about the v26 update! :D

@yandeu
Copy link
Owner

yandeu commented May 20, 2021

Granted subpath patterns was introduced in Node 12.20 / 14.13 w/ subpath folder mappings being the older way to handle things.

I will have a closer look at this topic in the future.

@yandeu
Copy link
Owner

yandeu commented May 28, 2021

I made some tests and have decided to completely drop CommonJS in January 2022 and will only support Node.js >=15.

@typhonrt
Copy link
Author

Anything in particular that is triggering this? If you're not targeting CommonJS I assume you'll target ESM? If so Node >= 14 is a fair balance. The only thing you'd have to consider is that importing named exports from a CommonJS package isn't supported until Node 14.13.0.

I'm having plenty of success releasing straight ESM modules Node 12.0.0+ w/ Github Actions testing across Win/Ubuntu/MacOS on a large spread from 12.0 - 16.x.

@yandeu
Copy link
Owner

yandeu commented May 28, 2021

assume you'll target ESM

Yes.


I just don't like hacks. And these ESM wrappers look a lot like hacks.

Also, it is just too much for me. TypeScript, ESM, CommonJS; Node.js 12, 14 ,15 ,16...

I guess a better setting would be "^14.8 || >=16"?
Support >v14.8 / drop v15 / support v16 an higher

@typhonrt
Copy link
Author

typhonrt commented May 28, 2021

14.13 is the sweet spot as that is when named exports for CommonJS packages works for ESM imports how you'd expect it.

It really wouldn't be hard to support Node 12.17+. That is when the experimental flag is dropped. Then you just have to keep in mind the CommonJS named export thing until 14.13.

As mentioned with not too much trouble Node 12+ can be supported. The trick is running ESM tests Node 12.0-12.17. You can check out this GH action I use for CI/CD. I install the esm package just for tests on Node 12.0, but in CI/CD only.

Also just a tip.. Node 12+ supports private class fields which is nice as that wasn't officially released yet.

Node 12.2+ supports createRequire.

Node 14+ supports optional chaining which can be quite useful.

@yandeu
Copy link
Owner

yandeu commented May 28, 2021

keep in mind the CommonJS named export thing until 14.13

My mistake. I wanted to write ^14.15. This is when the LTS (Fermium) started.

v12 reaches its end of support in April 2022, just a few months after I want to make the switch for most packages. five-server probably sooner.

Also "^14.15 || >=16" is easier to read than "^12.17 || ^14.15 || >=16" 😄

@yandeu
Copy link
Owner

yandeu commented May 28, 2021

I have just published my first ESM-only package called @yandeu/express-dev.

It works great. But:
Is there a better way to import ESM inside a .cjs file?
Do you have any experience with this?

The code below works:
Got it from the Node.js 14 docs.

// server.cjs

const express = require('express')
const app = express()
const port = 3000

const main = async () => {
  const ExpressDev = await import('@yandeu/express-dev')
  const { ExpressListen } = ExpressDev

  app.get('/', (req, res) => {
    res.send('Hello World!')
  })

  const listen = new ExpressListen(app)
  const openBrowser = false

  listen.listen(port, openBrowser).then(port => {
    console.log(`Running on port ${port}`)
  })

  setTimeout(async () => {
    await listen.kill()
  }, 5000)
}

main()

@typhonrt
Copy link
Author

Yep.. That's the way to do it. Right on.. Welcome to the ESM fold! :D Nice thing w/ targeting Node 14+ is that you can set things to ES2019.

@yandeu
Copy link
Owner

yandeu commented May 28, 2021

Nice, thanks!

I think I will also add the badge ES Modules (or similar) 😊

@typhonrt
Copy link
Author

Good idea! I'll have to look into that myself! Heh.. You might get a kick.. Though I'm creating pure ESM packages I figured out a fairly automated way to get good TS defs for the public API, so packing TS def files for my packages without using TS for dev.

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