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

Fix package exports for vitest #9332

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

Conversation

smeng9
Copy link
Contributor

@smeng9 smeng9 commented Oct 4, 2023

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

@smeng9
Copy link
Contributor Author

smeng9 commented Oct 11, 2023

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!

Copy link
Collaborator

@djhi djhi left a 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 👍

@smeng9
Copy link
Contributor Author

smeng9 commented Oct 12, 2023

Rebased with new package i18next introduced since 4.15

@djhi
Copy link
Collaborator

djhi commented Oct 13, 2023

Unfortunately this seems to break remix integration. We'll investigate to make sure it works in all cases

@slax57
Copy link
Contributor

slax57 commented Oct 13, 2023

FTR, the error we get with Remix:

❯ npm run dev

> dev
> remix dev --manual


 💿  remix dev

 info  building...
 info  built (11.4s)
file:///home/slax57/workspaces/remix-admin/build/index.js?t=1697200739546.5735:200
import { Admin, Resource, ListGuesser } from "react-admin";
         ^^^^^
SyntaxError: Named export 'Admin' not found. The requested module 'react-admin' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

import pkg from 'react-admin';
const { Admin, Resource, ListGuesser } = pkg;

    at ModuleJob._instantiate (node:internal/modules/esm/module_job:124:21)
    at ModuleJob.run (node:internal/modules/esm/module_job:190:5)

@smeng9
Copy link
Contributor Author

smeng9 commented Oct 13, 2023

I have checked why Remix is failing to parse the esm module and use the cjs. This can be fixed by adding a {type: module} field in the package.json inside the esm build folder.

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?

@smeng9
Copy link
Contributor Author

smeng9 commented Oct 8, 2024

Hi @slax57

Remix recently updated their template.
I tested using new template as long as we disable the ssr and use spa mode https://remix.run/docs/en/main/guides/spa-mode#usage we will not run into issues with this merge request.

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

@smeng9 smeng9 marked this pull request as ready for review October 8, 2024 04:02
@slax57 slax57 self-requested a review October 17, 2024 15:29
@djhi
Copy link
Collaborator

djhi commented Dec 19, 2024

Thanks for the PR.

I tested using new template as long as we disable the ssr and use spa mode https://remix.run/docs/en/main/guides/spa-mode#usage we will not run into issues with this merge request.

Unfortunately this is not good enough. It currently works in Remix, with SSR and without SPA mode.

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.

3 participants