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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(just-cartesian-product): enable esm #315

Merged
merged 11 commits into from Oct 31, 2021
Merged

feat(just-cartesian-product): enable esm #315

merged 11 commits into from Oct 31, 2021

Conversation

raulfdm
Copy link
Contributor

@raulfdm raulfdm commented Oct 23, 2021

Related

General

Enables ESM for all "just" packages.

The PR itself looks huge but that's because I have to generate a index.esm.js and tweak package.json from every single package.

Description

More and more we're migrating to ESM environment and it's important to, at least in this transition moment, support both.

This PR adds Rollup and does a transpiles the original entry point (written in CJS) into a new index.esm.js file.

The approach chosen was suggested here and as reference, used the suggestion from Dr. Axel Rauschmayer post: Hybrid packages (ESM and CommonJS).

The first attempt was using ESBuild, but after looking the outcome code and a suggestion (here, I've moved to Rollup + commonjs plugin.

Test

To see if works as expected, I've publish the package just-cartesian-product into a local npm proxy using Verdacio (more details of how to do that here). Then, I've published that package (with both CJS and ESM) to there:

Screenshot 2021-10-23 at 17 38 00

After that, I've created a brand-new vanilla-ts project using Vite which is well-known to push ESM ecosystem, install the just-cartesian-product from my local registry, copied the code from the README and console it:

Screenshot 2021-10-23 at 17 41 21

Screenshot 2021-10-23 at 17 41 06

After that, I run yarn build to see if everything would be picked correctly and run a served the outcome with a web server (live-server):

Screenshot 2021-10-23 at 17 43 32

Screenshot 2021-10-23 at 17 43 26

Worked like a charm! 馃暫

Overall Publishing

After I add index.esm.js,add info on each package's json, and commit, I've ran:

yarn lerna publish  --yes major --registry=http://0.0.0.0:4873/

This automatically bumped all versions to major and publish to my local registry:

Screenshot 2021-10-24 at 07 46 59

Random GIF

Dab!

Misc.

Script used to update package.json's
const glob = require('glob')
const fs = require('fs')
const path = require('path')

glob('packages/**/package.json', (er, filePaths) => {
  for (let filePath of filePaths) {
    const result = {};
    const {
      name,
      version, description,
      main,
      types,
      scripts,
      ...rest
    } = require(path.resolve(__dirname, filePath))


    result.name = name
    result.version = version;
    result.description = description;
    result.type = 'commonjs',
      result.main = main;
    result.module = 'index.esm.js',
      result.exports = {
        ".": {
          "require": "./index.js",
          "default": "./index.esm.js"
        }
      }

    if (types) {
      result.types = types
    }
    result.scripts = { ...scripts, build: 'rollup -c' }

    const finalContent = { ...result, ...rest }

    fs.writeFileSync(filePath, JSON.stringify(finalContent, null, 2))
  }
});

"eslint": "6.8.0",
"lerna": "3.20.2",
"tap-spec": "5.0.0",
"tape": "^4.0.0",
"typescript": "^4.2.3"
},
"scripts": {
"build": "lerna run build --stream $@",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this command allow us to run:

# for all packages
yarn build

# or filtering by package name
yarn build --scope=just-cartesian-product

@PuruVJ
Copy link
Contributor

PuruVJ commented Oct 23, 2021

Just a comment: native use rollup, because rollup conversation s cj's to esm directly, whereas esbuild is inserting to much stuff, which would also affect tree shakeavility(experienced firsthand this week 馃ゲ)

@raulfdm
Copy link
Contributor Author

raulfdm commented Oct 23, 2021

Just a comment: native use rollup, because rollup conversation s cj's to esm directly, whereas esbuild is inserting to much stuff, which would also affect tree shakeavility(experienced firsthand this week 馃ゲ)

@PuruVJ Hmm... Well, I've never had problems with that actually. Also, I'm seeing more and more people using ESBuild but indeed, the output goes differently from rollup.

Can you elaborate the problem you had? I'm genuinely curious to understand more.

By the way, I've also added rollup to provide a comparison.

@PuruVJ
Copy link
Contributor

PuruVJ commented Oct 23, 2021

https://www.npmjs.com/package/all-of-just

I authored this library just this week. I started with esbuild, but soon realised the output size was bigger, and also had side effects, so bundless wouldn't tree shake. So I had to move to rollup, and it effortlessly converts cj's to esm without introducing any extra functions with side effects.

It's slower, but it's worth it to keep the library as small as possible

@angus-c angus-c added the WIP label Oct 23, 2021
@raulfdm raulfdm marked this pull request as ready for review October 24, 2021 05:50
@raulfdm raulfdm changed the title WIP: feat(just-cartesian-product): enable esm feat(just-cartesian-product): enable esm Oct 24, 2021
@raulfdm
Copy link
Contributor Author

raulfdm commented Oct 24, 2021

@angus-c it's ready to be reviewed. The pipeline is failing because nvm list (??). Also, if you want I revert the bump version so you can do once it's merged, please let me know.

@PuruVJ I've replaced ESBuild with Rollup + CommonJS plugin. Thanks for the insight.

@angus-c
Copy link
Owner

angus-c commented Oct 24, 2021

Really appreciate your efforts here! Apologies you're going to have be patient with me, I have a lot of other things on my plate right now and want to do due diligence on this review. So it's going to be a while.

@raulfdm
Copy link
Contributor Author

raulfdm commented Oct 24, 2021

No problem @angus-c ... take your time. I don't think anyone will die without that! 馃槃

Whenever you have some time you review and, if you want, merge straight away!

@angus-c
Copy link
Owner

angus-c commented Oct 30, 2021

Hi Raul, this looks great. I'm excited. Tests are passing now. Can you apply this diff to your forked repo to resolve conflicts?

Also, question: do you think we need major version bumps? AFAICT there will be no compatibility changes for commonJS users, so minor bump should be ok?

Then I think we're good to go!

@raulfdm raulfdm closed this Oct 31, 2021
@raulfdm raulfdm reopened this Oct 31, 2021
@raulfdm
Copy link
Contributor Author

raulfdm commented Oct 31, 2021

@angus-c I've reverted the version bump and manage the conflicts.

Since I can't merge this PR, whenever you feel it's ok, go ahead. I'd recommend you to instead merge, square and merge you can you have a single commit message and add "BREAKING CHANGE" so people can be aware that's a major (and why).

Also you'll need to run lerna version by yourself.

Anyhow, thanks mate! :)

@angus-c
Copy link
Owner

angus-c commented Oct 31, 2021

Thank you!

(I still think it's not breaking change :) Instead it's minor version bump since acting users can upgrade without anything changing, but it adds a new feature (esm flavor))

@angus-c angus-c merged commit f6076d1 into angus-c:master Oct 31, 2021
@PuruVJ
Copy link
Contributor

PuruVJ commented Oct 31, 2021

@angus-c it'd be great to publish all these to npm. Some of em were last published 10 months ago but updated very recently 馃ゲ馃ゲ

@angus-c angus-c removed the WIP label Oct 31, 2021
@angus-c
Copy link
Owner

angus-c commented Oct 31, 2021

@PuruVJ curious which those were, I try to publish everything as soon as it's updated. Anyway I published every package just now :)

@angus-c
Copy link
Owner

angus-c commented Oct 31, 2021

@raulfdm ok all landed and published. Seems to work perfectly.
Will tweet about it tomorrow!

@rqbazan rqbazan mentioned this pull request Oct 31, 2021
return {
input: resolve(path, './index.js'),
output: {
file: resolve(path, './index.esm.js'),

Choose a reason for hiding this comment

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

This extension does not work for NodeJS' native esm implementation, the extension needs to be .mjs for NodeJS to run the file in ESM mode.

Right now when using NodeJS' native esm implementation, an error will be thrown when using import on any Just packages:

(node:55016) Warning: To load an ES module, set "type": "module" in the package.json or use the .mjs extension.
/tmp/just-esm-example/node_modules/just-camel-case/index.esm.js:40
export {stringCamelCase as default};
^^^^^^

SyntaxError: Unexpected token 'export'
    at Object.compileFunction (node:vm:352:18)
    at wrapSafe (node:internal/modules/cjs/loader:1031:15)
    at Module._compile (node:internal/modules/cjs/loader:1065:27)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1153:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/translators:196:29)
    at ModuleJob.run (node:internal/modules/esm/module_job:183:25)
    at async Loader.import (node:internal/modules/esm/loader:178:24)

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.

Use ES Modules with type:module
4 participants