fix: Add type module in package.json

ArthurGoupil commented 6 days ago

Description

There is a missing type: module in package.json making jest be lost when trying to use esm/cjs.

Type of Change

[x] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
[ ] Documentation update

Checklist

Before submitting your pull request, please make sure the following is done:

[x] I have read the CONTRIBUTING doc
/ I have added tests that prove my fix is effective or that my feature works
/ I have added necessary documentation (if appropriate)
[x] I have followed the coding guidelines in this project
[x] I have tested my changes on different browsers and screen sizes

Linked Issues

No posted issue

Test Plan

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.

Screenshots (if appropriate)

Include screenshots or GIFs if relevant. This is especially important for UI-related changes.

Further Comments

If you have any additional comments or questions, please add them here.

gpbl commented 4 days ago

Thanks! Now is docusaurus (so, webpack) to fail with:

Module not found: Error: Can't resolve './' in '/home/runner/work/react-day-picker/react-day-picker/dist/esm'
Did you mean 'index.js'?
BREAKING CHANGE: The request './' failed to resolve only because it was resolved as fully specified
(probably because the origin is strict EcmaScript Module, e. g. a module with javascript mimetype, a '*.mjs' file, or a '*.js' file where the package.json contains '"type": "module"').
The extension in the request is mandatory for it to be fully specified.

My understanding is that we need to update also the import statements to use index.js and the file extensions. How do we do this with the TypeScript compiler?

guillaumewuip commented 3 days ago

Hello there! Jumping on this to help unlock this esm/cmjs issue.

What's the issue exactly here?

we're bundling the code with raw tsc (tsc --project tsconfig-esm.json)
with the current tsconfig.json config, we're not forcing the import/export paths to be fully specified (ie. my/path/index.js instead of my/path).
tsc as no ability to transform the path in to fully specified version at build time (see for example this issue)
so files in the dist/esm output dir don't have fully specified import/export paths, and this is an issue for tools that want to leverage strict ESM imports (here: docusaurus)
with "type": "module", tools that can leverage ESM are more likely to do so. For example docusaurus is now trying to import the ESM files.

In the ECMAScript spec, they mention that imports need to have a file extension, so import { foo } from './foo'; is not valid (see in Node.js’s doc).

How to fix?

We need to output esm files that have fully specified imports. I think we have two options:

make all paths fully specified

Here we need to transform lines like import "./my/path/" into import "./my/path/index.js" in all source files (note the.jseven in.ts` source files.
add a bundler to do this fully specified transformation for us, for example rollup

No change to do on the source files. We will let the bundler do this for us.

Are you opened in adding a bundler like rollup to output common js and esm files @gpbl? We can propose a PR for that with @ArthurGoupil

gpbl commented 3 days ago

Hi @guillaumewuip, thanks for the suggestions!

I really prefer adding the extension to the import rather than introducing rollup.js again. There should be an extension for eslint as well.

guillaumewuip commented 3 days ago

Ok, so the next steps is to update this PR by updating each import lines, right?

Basically @ArthurGoupil :

-import {} from './toto'
+import {} from './toto.js'

-import {} from './tata'
+import {} from './tata/index.js'

gpbl commented 3 days ago

Ok, so the next steps is to update this PR by updating each import lines, right?

Yes - that should work for everyone.

ArthurGoupil commented 3 days ago

@gpbl I think there is a blocker here, looks like your testing environment doesn't accept the explicit .js imports

gpbl commented 3 days ago

@ArthurGoupil thanks for looking at it , and sorry for the hassle. Will check it out asap.

gpbl commented 2 days ago

@ArthurGoupil it looks like now it pass 🎉

How to add a test to the build pipeline to avoid issues like these? 🤔

Could you maybe try to install main branch

npm install git+https://github.com/gpbl/react-day-picker.git

and confirm it works now for you? Thanks.

ArthurGoupil commented 2 days ago

Thanks for the updates @gpbl ! I'll test shortly and tell you if I still have issues. But it should be alright now.

gpbl / react-day-picker