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 support for react-admin v5 #572

Merged
merged 18 commits into from
Jul 11, 2024
Merged

Add support for react-admin v5 #572

merged 18 commits into from
Jul 11, 2024

Conversation

fzaninotto
Copy link
Contributor

@fzaninotto fzaninotto commented Jul 2, 2024

  • Upgrade dependencies
  • Make it compile
  • Make the tests pass
  • use the latest react-admin features

Closes #569

};
/* eslint-enable tree-shaking/no-side-effects-in-initialization */

const AdminGuesserWithError = ({
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We no longer need this wrapper as react-admin v5 has a global error boundary

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

component extracted to its own file for clarity

@@ -202,51 +121,4 @@ const AdminGuesser = ({
);
};

/* eslint-disable tree-shaking/no-side-effects-in-initialization */
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

react-admin v5 no longer has propTypes, React 18.3 warns against them, so I removed them

* If child components are passed (usually `<ResourceGuesser>` or `<Resource>` components, but it can be any other React component), they are rendered in the given order.
* If no children are passed, a `<ResourceGuesser>` component is created for each resource type exposed by the API, in the order they are specified in the API documentation.
*/
export const AdminResourcesGuesser = ({
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extracted to its own file for clarity

SchemaAnalyzer,
} from './types.js';

const ResourcesIntrospecter = ({
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to its own file for clarity

@fzaninotto
Copy link
Contributor Author

fzaninotto commented Jul 3, 2024

I'm blocked.

For a reason I don't understand, the jest tests fail at reading the QueryClient from the QueryClientProvider.

I've verified that:

  • there is only one version of @tanstack/react-query and the related core library in the project
  • we no longer have any dependency on react-query in the code
  • there is only one version of react and react-dom
  • the tests still fail if I force the resolution of @tanstack/react-query to the very same version that is in react-admin (5.8.4)
  • the tests still fail if I extract the new QueryClient() out of the test function
  • the problem still occurs with a more recent yarn version (4.0.2)

If someone wants to give it a try, be my guest.

That being said, the storybook runs the test admin with greetings, so we're probably close.

Comment on lines +212 to +213
const embeddedsField = await screen.findByLabelText(
'resources.users.fields.embeddeds',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is due to a change in RA v5: ArrayInputs with scalar value no longer have a label of the form resources.users.fields.embeddeds.0. The recommendation is now to provide your own label (which I did).

Comment on lines +12 to +13
'^@tanstack/react-query$':
'<rootDir>/node_modules/@tanstack/react-query/build/modern/index.cjs',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason, Jest resolved imports to @tanstack/react-query to the ESM code when imported from the test file, and to the CJS code when imported from ra-core. This caused the issue about QueryClient because the references to the context object were not equal.

This line forces the resolution to the CJS code all the time, which solves the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it a problem react-admin users may be facing, too?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed it could be, if they are using Jest with NODE_OPTIONS=--experimental-vm-modules. But I'm not sure it's react-admin's responsibility to document this, as this rather looks like a Jest bug to me...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved to core

@fzaninotto fzaninotto changed the title [WIP] Add support for react-admin v5 Add support for react-admin v5 Jul 10, 2024
@fzaninotto fzaninotto added the RFR label Jul 10, 2024
src/create/CreateGuesser.tsx Outdated Show resolved Hide resolved
src/edit/EditGuesser.tsx Outdated Show resolved Hide resolved
src/field/FieldGuesser.tsx Outdated Show resolved Hide resolved
src/list/FilterGuesser.tsx Outdated Show resolved Hide resolved
src/list/ListGuesser.tsx Outdated Show resolved Hide resolved
src/show/ShowGuesser.tsx Outdated Show resolved Hide resolved
Co-authored-by: Gildas Garcia <1122076+djhi@users.noreply.github.com>
.dockerignore Outdated Show resolved Hide resolved
package.json Outdated
@@ -1,6 +1,6 @@
{
"name": "@api-platform/admin",
"version": "3.4.8",
"version": "4.0.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To revert, this will be handled by yarn version during the release process: https://github.com/api-platform/admin/blob/main/CONTRIBUTING.md#tag-a-new-version-contributors-only

Co-authored-by: Kévin Dunglas <kevin@dunglas.fr>
@slax57 slax57 merged commit 2027186 into main Jul 11, 2024
4 checks passed
@fzaninotto
Copy link
Contributor Author

Great! Can someone with NPM credentials trigger the publication of the package?

@fzaninotto fzaninotto deleted the react-admin-v5 branch July 12, 2024 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support react-admin v5
4 participants