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

Add exports to package.json in all packages #9457

Open
wants to merge 4 commits into
base: next
Choose a base branch
from

Conversation

otaviosoares
Copy link

@otaviosoares otaviosoares commented Nov 18, 2023

fixes #9452

@djhi
Copy link
Contributor

djhi commented Nov 18, 2023

Thanks! Isn't it a duplicate of #9332?

@smeng9
Copy link
Contributor

smeng9 commented Nov 18, 2023

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 export * from './Admin'; without .js extension. While vite is smart enough to iterate through all possible extensions and find the file, remix is not clever.

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 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:"

@djhi
Copy link
Contributor

djhi commented Nov 18, 2023

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 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:"

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

@smeng9
Copy link
Contributor

smeng9 commented Nov 18, 2023

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?

@otaviosoares
Copy link
Author

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 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?

@smeng9
Copy link
Contributor

smeng9 commented Nov 18, 2023 via email

@otaviosoares
Copy link
Author

otaviosoares commented Nov 20, 2023

Do you mean we have to change how we build and release react-admin, switching from tsc to vite or esbuild?

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.

@smeng9
Copy link
Contributor

smeng9 commented Nov 20, 2023

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.

@smeng9
Copy link
Contributor

smeng9 commented Nov 20, 2023

Hi @otaviosoares

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.

@djhi
Copy link
Contributor

djhi commented Nov 20, 2023

tsup won't do. We can't have TS source maps with it

@otaviosoares
Copy link
Author

Added a few changes to address the points you raised:

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.

Disabled minifying and bundling in tsup. Using a glob as entrypoint.

tsup won't do. We can't have TS source maps with it

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
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.

None yet

3 participants