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

Support node v13 with es modules #164

Merged
merged 3 commits into from May 1, 2020
Merged

Conversation

vikerman
Copy link
Contributor

@vikerman vikerman commented Apr 18, 2020

Creates conditional export that points "import" conditional exports to mjs files. This lets node load the esm version without having to set "type": "module" on the main package.json.

This is similar to the approach in the main preact repo - preactjs/preact#2451

@jkrems
Copy link

jkrems commented Apr 19, 2020

One note of caution is that this is a breaking change: People who currently require or import ./dist/ paths directly would no longer be able to. This can be softened by adding an export of “./“.

Excited to see this added to htm!

"import": "./dist/htm.mjs",
"require": "./dist/htm.js",
"browser": "./dist/htm.module.js",
"umd": "./dist/htm.umd.js"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a good opportunity to consider dropping umd for the export map? Or perhaps in a separate PR.

I'm not sure I see the value in specifying a umd format output in export maps.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just following the pattern from the main preact repo. I can do the change if needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, I think this is a good followup item to consider. Not blocking for this PR.

Thanks for doing the work!

@vikerman
Copy link
Contributor Author

vikerman commented Apr 28, 2020

Looks like the node version changed and the test is failing because npm cache has older version of compiled iltorb. Clearing the npm cache in Travis could help

@developit
Copy link
Owner

@vikerman - just switched to Actions, hopefully that addresses it.

@developit
Copy link
Owner

Everything looks good. I'm wondering if there's a way to test this though.
Something like:

{
  "scripts": {
    "test:dist": "npm pack && mv htm*.tgz test/fixtures/dist/htm.tgz && cd test/fixtures/dist && node ."
  }
}

... where dist/ would be:

// package.json
{ "dependencies": {"htm": "file:htm.tgz" } }
// index.mjs
import 'htm';
import 'htm/preact';
import 'htm/preact/standalone';
import 'htm/react';

Creates conditional export that points "import" conditional exports to mjs files. This lets node load the esm version with having to set "type": "module" on the main package.json.

This is similar to the approach in the main preact repo - preactjs/preact#2451
@vikerman vikerman force-pushed the master branch 2 times, most recently from 2adbe99 to 4603f22 Compare April 30, 2020 02:38
@vikerman
Copy link
Contributor Author

vikerman commented Apr 30, 2020

Added the test. It's not hooked up to CI though. Would work for node versions >=13. [Edit: It's hooked up to the CI now]

@vikerman
Copy link
Contributor Author

Hooked up the ESM dist test to CI when node version is 14.x.

@developit
Copy link
Owner

Wow, thanks @vikerman that is absolutely perfect!

@developit developit merged commit 452e1b3 into developit:master May 1, 2020
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

4 participants