Skip to content

Fix package exports for vitest#9332

Closed
smeng9 wants to merge 7 commits intomarmelab:masterfrom
smeng9:fix-exports
Closed

Fix package exports for vitest#9332
smeng9 wants to merge 7 commits intomarmelab:masterfrom
smeng9:fix-exports

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
Contributor

@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
Contributor

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
Contributor

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.

@fzaninotto
Copy link
Member

Is this still relevant after #10453?

@smeng9
Copy link
Contributor Author

smeng9 commented Jan 17, 2025

Hi @fzaninotto

This merge request can be closed. Now the browser package can be tested natively in a browser.

React-Admin is currently a client package not a server package. Previously vitest mocks the browser environment (like DOM and module imports) in a headless server using node module resolution.

However, if at some point we want to run react-admin in server #9452 #9457 without disabling ssr (

To do that, import the `<AdminApp>` component in Next.js by using [lazy loading](https://nextjs.org/docs/pages/building-your-application/optimizing/lazy-loading) and specify the [`ssr` option to false](https://nextjs.org/docs/pages/building-your-application/optimizing/lazy-loading#with-no-ssr).
and
+ ssr: {
) we may still need to update how the package is published at a later time.

@fzaninotto
Copy link
Member

Understood, thanks for the clarification.

@fzaninotto fzaninotto closed this Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants