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

rename esm modules to .mjs #359

Merged
merged 1 commit into from
Nov 9, 2021
Merged

rename esm modules to .mjs #359

merged 1 commit into from
Nov 9, 2021

Conversation

angus-c
Copy link
Owner

@angus-c angus-c commented Nov 7, 2021

No description provided.

@angus-c
Copy link
Owner Author

angus-c commented Nov 8, 2021

@PuruVJ @raulfdm @brandonryan @JakeChampion @rqbazan @nosovk

OK I've renamed all ESM modules to have .mjs suffix. This should make esm modules recognizable to NodeJS, sveltekit, NextJS and others consumers.

Unless there are objections I'm going to merge this.

Remaining optimizations that I can see we might want:

  • Add ./package.json to exports add ./package.json to exports #343
  • Switch to manually writing esm modules and using rollup to auto-generate CJS (as per @PuruVJ ESM -> CJS generation is more reliable than vice versa)

Feedback welcome! I probably missed something :)

@angus-c angus-c removed the WIP label Nov 8, 2021
@angus-c angus-c changed the title WIP: rename esm modules to .mjs rename esm modules to .mjs Nov 8, 2021
@raulfdm
Copy link
Contributor

raulfdm commented Nov 8, 2021

Hey @angus-c .

I've created a repository that can help you to test within different apps: https://github.com/devraul/just-test

I tried to write a minimum README so you can understand how to do.

Take a look and let me know if there's anything else I can help you with.

@angus-c
Copy link
Owner Author

angus-c commented Nov 8, 2021

Thanks @raulfdm, will take a look tomorrow.

@angus-c
Copy link
Owner Author

angus-c commented Nov 9, 2021

Hey @raulfdm, love just-test! I'm going to link it from CONTRIBUTING.md.
BTW I didn't need verdaccio, I just used yarn pack to build a tarball and then yarn add <tarball.tgz> in just-test.

All apps passed with this PR (rename esm modules to .mjs) except both sveltekit tests failed.

pnpm run test-just --filter sveltekit-static

> [email protected] test-just /Users/anguscroll/Documents/github/just-test/apps/sveltekit-static
> svelte-kit build

file:///Users/anguscroll/Documents/github/just-test/node_modules/.pnpm/@[email protected][email protected]/node_modules/@sveltejs/kit/dist/cli.js:856
                        https = https || !!vite_config.server?.https;
                                                              ^

SyntaxError: Unexpected token '.'
    at Loader.moduleStrategy (internal/modules/esm/translators.js:140:18)
/Users/anguscroll/Documents/github/just-test/apps/sveltekit-static:
 ERR_PNPM_RECURSIVE_RUN_FIRST_FAIL  [email protected] test-just: `svelte-kit build`
Exit status 1

Not sure what the issue is here.

When I used [email protected] (current master), five of the apps in just-test failed (node-vanilla-esm, node-vanilla-mjs, nuxt-2-project and the two sveltekit apps).

So bottom line, I'm going to land and publish this PR 😀

@PuruVJ
Copy link
Contributor

PuruVJ commented Nov 9, 2021

just-diff is still exporting an object as default, so it won't work 🥲

@angus-c
Copy link
Owner Author

angus-c commented Nov 9, 2021

just-diff is still exporting an object as default, so it won't work 🥲

https://github.com/angus-c/just/blob/master/packages/collection-diff/index.mjs#L146

@PuruVJ
Copy link
Contributor

PuruVJ commented Nov 9, 2021

Ohhh my bad 😅

@PuruVJ
Copy link
Contributor

PuruVJ commented Nov 9, 2021

Maybe it;s how we're exporting in package.json that's an isssue. Maybe have a look at how I am doing it in all-of-just? https://github.com/PuruVJ/all-of-just/blob/main/package.json

PS: this package.json ends up in dist, so that's why paths are relative to dist only

@angus-c
Copy link
Owner Author

angus-c commented Nov 9, 2021

@PuruVJ just to be clear, I'm not sure there is a problem with this PR. It passes in every app type in @raulfdm's just-test except sveletekit. Need someone to test it in RL sveltekit instance to confirm whether this is an issue. Regardless landing this PR made a big improvement across the app ecosystem and I don't want to rush to make patches for one app type if they will break other apps or compromise established standards.

The only difference I see with all-of-just is your export statement includes module and import properties while this PR uses default. According to node spec default is the fallback for all non-mentioned properties so this should work the same or better.

@raulfdm
Copy link
Contributor

raulfdm commented Nov 9, 2021

@angus-c about testing in a real Sveltekit, #332 (comment) check this comment. This "MailCheck" website uses Sveltekit and a particular "just-*" package.

What you can do to see in is cloning this repository inside just-test/apps/, checking which just package it uses, try the .mjs fix and see if the build happens. If not then we have to take a closer look if there's a problem with Sveltekit + Vite (I know there are some issues there related to modules but I'm not sure if it's the same).

@angus-c
Copy link
Owner Author

angus-c commented Nov 9, 2021

Thanks @raulfdm. Will check when I have a chance (also this PR is now published so I will ask the app author directly)

@nosovk
Copy link
Contributor

nosovk commented Nov 9, 2021

@angus-c @raulfdm
MailCheck-co/mailcheck.site@f0581d6
I've updated to latest version and it seems that all works in proper way in SvelteKit for now.

@angus-c
Copy link
Owner Author

angus-c commented Nov 9, 2021

@nosovk great, thanks for letting me know

@angus-c angus-c deleted the mjs branch November 20, 2021 07: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.

4 participants