-
-
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
Add exports to package.json in all packages #9457
base: next
Are you sure you want to change the base?
Conversation
Thanks! Isn't it a duplicate of #9332? |
I think it's a duplicate. #9332 fixes vitest but creates an another problem for remix after we add the exports field in package.json to force remix to resolve to an esm package instead of cjs. The reason of failure is remix's file resolution algorithm for esm packages requires specific file extension and package type during compile. If you go to https://www.npmjs.com/package/react-admin?activeTab=code react-admin/dist/esm/index.js and you can find it is Now we may have two options:
|
Vite is already available in Remix v2, although flagged as unstable. I'm using it on another project without any issues. Haven't tested react-admin with it though |
Oh I don't know the already support it in unstable. However shall we update the remix tutorial to recommend users to use unstable remix plugin for vite? |
what about using |
Do you mean we have to change how we build and release react-admin, switching from tsc to vite or esbuild?
Get Outlook for iOS<https://aka.ms/o0ukef>
…________________________________
From: Otávio Augusto Soares ***@***.***>
Sent: Saturday, November 18, 2023 5:31:54 PM
To: marmelab/react-admin ***@***.***>
Cc: Meng, Shaoyu ***@***.***>; Comment ***@***.***>
Subject: Re: [marmelab/react-admin] Add exports to package.json in all packages (PR #9457)
Now we may have two options:
1. Add exports field, then add {"type": "module"} to dist/esm/package.json, then add *.js to all files in the source code during exports, then add *.js to all files regarding lodash and @mui/icon-material imports, then fix the import of @mui/material/styles, then fix react-router by importing the whole package and then unpack it since remix cannot correctly resolve default exports and we are done. Both remix and vitest can be happy.
2. Wait an undisclosed amount of time or another year for remix to release v3 as they announced in their blog post https://remix.run/blog/remix-heart-vite<https://urldefense.com/v3/__https://remix.run/blog/remix-heart-vite__;!!DZ3fjg!5TvQ_2wCoL20uABgjbOpdQ9wINxG5AtzXolTSqeXj2lISlQLjVlm2dD6khr6Fqd6QQX1vKsQ7LtSEkAq7-_vlP4v0ME$> they don't want to "be building a worse version of Vite", and "In fact, with Vite, Remix is no longer a compiler. Remix itself is just a Vite plugin:"
what about using moduleResolution: bundler in tsconfig.json and bundle the code using vite or esbuild to avoid having to add file extensions to imports?
—
Reply to this email directly, view it on GitHub<https://urldefense.com/v3/__https://github.com/marmelab/react-admin/pull/9457*issuecomment-1817676672__;Iw!!DZ3fjg!5TvQ_2wCoL20uABgjbOpdQ9wINxG5AtzXolTSqeXj2lISlQLjVlm2dD6khr6Fqd6QQX1vKsQ7LtSEkAq7-_v2JAgT6k$>, or unsubscribe<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AJHAEC2BIIK4A6B2Z6VHIP3YFFAOVAVCNFSM6AAAAAA7Q25KBOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJXGY3TMNRXGI__;!!DZ3fjg!5TvQ_2wCoL20uABgjbOpdQ9wINxG5AtzXolTSqeXj2lISlQLjVlm2dD6khr6Fqd6QQX1vKsQ7LtSEkAq7-_vPh41Idk$>.
You are receiving this because you commented.Message ID: ***@***.***>
|
Build yes, release can stay the same. Changes are minimal, though. I gave it a try with tsup (esbuild) and it seems to work nicely. Let me know your thoughts. I can update this PR. |
As long as we do not bring back the regression here #7580 that bundles everything using tsup which makes it hard to debug and do module federation it should be fine. |
I have checked how the team did using tsup in 4.0.x, Seems they have tried to mark react-hook-form as external https://github.com/marmelab/react-admin/blob/59f8e3dfd20b3ee9c6958068d091894a73feef63/packages/ra-core/package.json#L23C19-L23C50 so it can avoid duplicate instance of react-hook-form being bundled which breaks the FormContext singleton. One example duplicate package import happens when we tried to use ra-no-code in module federation, which has a shadow dependency of react-hook-form through the usage of SimpleForm from react-admin and tsup did not exclude it before this flag was added. |
tsup won't do. We can't have TS source maps with it |
Added a few changes to address the points you raised:
Disabled minifying and bundling in tsup. Using a glob as entrypoint.
Using tsup's onSuccess + tsc emitDeclarationOnly to generate source maps. @smen9 react-hook-form should be picked up as external automatically since it's listed as a dependency / peerDependency. I'm going to try to reproduce the scenario where it's being bundled. |
Using tsup only to transpile ts files. Disabled bundling, minifying and declaration generation. Using tsup onSuccess to use tsc to emit declaration and declaration sourcemap
81e177f
to
77dd897
Compare
fixes #9452