-
-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Fix package exports for vitest #9332
base: master
Are you sure you want to change the base?
Conversation
Hi @djhi any chance that this can be reviewed and merged so we can pass the vitest https://stackblitz.com/edit/vitejs-vite-2etbdy?file=src%2Fposts.test.tsx here and start using vitest in our projects? Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me 👍
Rebased with new package i18next introduced since 4.15 |
Unfortunately this seems to break remix integration. We'll investigate to make sure it works in all cases |
FTR, the error we get with Remix:
|
I have checked why Remix is failing to parse the esm module and use the cjs. This can be fixed by adding a However then it comes with another issue: file extension. Remix does not have a resolver that can automatically infer file extensions like vite and webpack. TypeScript won't automatically add the extension to the import. It requires all the relative imports to add a .js extension to correctly resolve files. https://www.typescriptlang.org/docs/handbook/esm-node.html#type-in-packagejson-and-new-extensions E.g. in https://github.com/marmelab/react-admin/blob/master/packages/react-admin/src/index.ts#L1 would become export * from './Admin.js'; and apply the same thing for all the files in the code base. I believe this change is too large. What's your thought on this? |
Hi @slax57 Remix recently updated their template. This is because we use a lot of client side libraries, we should follow the same thing as https://marmelab.com/react-admin/NextJs.html try to put react-admin in a non ssr component or disable the ssr in config. Besides tests fail with react-hook-form context in #9332 (comment), we noticed more tests to fail with @tanstack/react-query after migrating to v5 |
Thanks for the PR.
Unfortunately this is not good enough. It currently works in Remix, with SSR and without SPA mode. |
Replaces #9309 and #9310
Main differences with previous merge requests are omitting the type field so we don’t need to rename .js file extensions to .cjs or .mjs
Also adds * to match arbitrary files in case some users directly import certain internal files in the package.
The reason for this package.json change is because vitest parses package entry points according to the latest node.js package entry point format here https://nodejs.org/api/packages.html#package-entry-points