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

Code converted to ESM, outputs both CJS and ESM #109

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

Conversation

yankeeinlondon
Copy link

  • All code converted to ESM base and then uses tsup to convert to minimal CJS and ESM exports.
  • All tests pass
  • Pointed benchmark to CJS output with result of: minimatch x 113,117 ops/sec ±0.69% (93 runs sampled)

@jimmywarting
Copy link

jimmywarting commented Aug 6, 2022

Great work! 👍
Would love to see this merged ❤️
Personally I would just have ditched cjs all together and make a major release.

@jonschlinkert
Copy link
Member

Sorry I haven't gotten around to merging this @yankeeinlondon. This is great work! It's a big PR, and I want to make sure I review before we merge and publish.

Personally I would just have ditched cjs all together and make a major release.

I feel your pain. I also look foward to getting rid of cjs stuff. However, I still quite a number of issues from people who can't migrate yet for one reason or another.

@yankeeinlondon
Copy link
Author

Yeah we'll all be happy and fat in 2 years time but there's a lot of transition pain at the moment. The one thing i'm uncertain of is if the CJS exports are working as they were as the base code is now ES based the question I have is does the default export behave the same as CJS's module.exports hash? I know the transpilation tools were tuned into ensuring a good mapping from CJS to ESM but not sure if enough TLC has gone into the reverse. TSUP is a high quality tool though so I kind of put my faith into that and all the tests passing.

Sorry, just did a day of travel from UK to Finland (and mainly live in LA these days) so my brain's a bit fried can't quite remember if the tests are run under CJS or ESM. If ESM, then we should probably put in a few sanity tests (or just double up the testing) with module systems.

BTW, I think I have a 70% done convergence to Typescript if you're interested.

@jonschlinkert
Copy link
Member

just did a day of travel from UK to Finland

I've always wanted to go to Finland. Hope you had a great trip!

does the default export behave the same as CJS's module.exports hash?

If I understand your question correctly: No, but it can.

Let's say you have the following:

// in numbers.js
export default { one: 1, two: 2 };

You cannot destructure one and two (the way you'd be able to do if it was a commonjs module.exports and require statements) since one and two are not directly exported. Meaning the following doesn't work:

// doesn't work
import { one, two } from './numbers.js';

However, destructuring works if you do this:

// in numbers.js
export const one = 1;
export const two = 2;

Now this will work:

import { one, two } from './numbers.js';

Even better (IMHO), if we also add a default export, like this:

// in numbers.js
export const one = 1;
export const two = 2;
export default { one, two };

Then we can do all of these:

import numbers from './numbers.js';
import { one, two } from './numbers.js';
import numbers, { one, two } from './numbers.js';

And of course all of the fun stuff with *.

Please forgive me if I misunderstood your question and just codesplained something you already knew.

@yankeeinlondon
Copy link
Author

I think it's fair to say I knew almost nothing in the state I was in last night :)

WRT to exporting a hash as the default export (alongside named exports), I agree that keeps useful optionality for the caller. I thought I'd done that actually but maybe this was just in my non-published TS branch. I guess I should just have a look but last night I had all these creative ideas in Rust dancing in my head (pre-wine) and right now I've got to address a planning issue where I forgot to bring my EU power adapter and the hotel's singular device was a fire hazard.

I think the most important thing is we just document expected CJS uses cases as tests. Some edge cases which might be harder to have this transpiled source address are things like this old gem:

const one = require("picomatch").one;

note: i'm not saying it is a problem but without testing explicitly I wouldn't be 100% sure. I also think it might be ok to stop supporting this syntax and force those who want to destructure to do it over two lines like:

const pico = require("picomatch");
const { one, two } = pico;

For a while tools like Mocha and Jest were causing a lot of friction for upstanding ESM people who had downstream dependencies which didn't have a CJS fallback to use but their use of the "import" syntax immediately limits the number of corner cases one has to consider. In fact, for me it was testing tools that held me back from fully jumping over to ESM a year or more ago but today I think Mocha and Jest have an ok story there don't they. I switched to Vitest and it's a ESM born tool so there's no issue there.

@yankeeinlondon
Copy link
Author

I'll try and squeeze in a hour over the next day or two and get a few tests in to validate some mainly CJS use cases. I'd like to help if I can but didn't want to push into something people weren't interested in (though it sounds more like people are just busy ... surprise, surprise). Picomatch and the match family have a huge footprint in tooling today and it would be nice to see it move into an ESM world where everything is candy canes and lollipops.

@yankeeinlondon
Copy link
Author

Sorry folks I haven't done the TODO I self-signed up for and I keep on keeping this tab open with the hope that @jonschlinkert may have done his review to give it a thumbs up. I am quite busy but will still try and validate the import syntax (and/or limitations) which pure CJS users will have when switching to an ESM implementation but I can't promise a timeline just yet.

@benmccann
Copy link

This is checking in two new compiled files under dist/. Perhaps those should be generated at build-time rather than version controlled?

@yankeeinlondon
Copy link
Author

It's been a hot minute since I posted this PR but I'd agree that general best practice is to exclude dist/* in the .gitignore file. Not sure if this repo had been operating under a different principle before (in the past this "best practice" was less practiced) and there remain a few edge cases where it's desirable to have the build artifacts in the repo itself.

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