micromatch / picomatch

Blazing fast and accurate glob matcher written JavaScript, with no dependencies and full support for standard and extended Bash glob features, including braces, extglobs, POSIX brackets, and regular expressions. Used by GraphQL, Jest, Astro, Snowpack, Storybook, bulma, Serverless, fdir, Netlify, AWS Amplify, Revogrid, rollup, routify, open-wc, imba, ava, docusaurus, fast-glob, globby, chokidar, anymatch, cloudflare/miniflare, pts, and more than 5 million projects! Please follow picomatch's author: https://github.com/jonschlinkert
https://github.com/micromatch
MIT License
972 stars 56 forks source link

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

Open yankeeinlondon opened 2 years ago

yankeeinlondon commented 2 years ago
jimmywarting commented 2 years ago

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

jonschlinkert commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 9 months ago

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

yankeeinlondon commented 9 months ago

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.