React-admin 3 #263

alanpoulain · 2020-01-03T08:57:06Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	yes
Deprecations?	no
Tickets	N/A
License	MIT
Doc PR	N/A

Supersede #257.

Upgrade the Admin for react-admin 3.

It will be a new 2.0 version because of the BC.

Things done:

Use hooks instead of classes.
AdminGuesser can be used instead of HydraAdmin (to make it clear it's not related to Hydra).
A custom resourceSchemaAnalyzer can be pass as a prop of AdminGuesser.
A custom history can be pass as a prop.
The Hydra data provider is using the new type.
Unused code removed and clean up.

alanpoulain · 2020-01-03T17:46:07Z

@fzaninotto It would be great if you could have a look 🙂

mleczakm · 2020-01-03T22:09:52Z

src/hydra/HydraAdmin.js

  authProvider,
+  i18nProvider,
+  history = createHashHistory(),


WDYT about making it configurable to support also browser history?

It's a prop so you can pass whatever you want (for example an instance of createBrowserHistory).

You should add info about this change to "Things done:" because it was not possible before :)

yudhir · 2020-01-05T18:30:17Z

Thanks, looking Forward .

Deuchnord · 2020-01-06T15:07:16Z

Tested on a fresh project after an npm uninstall @api-platform/admin && npm install --save alanpoulain/admin#ra3 && docker-compose up --build, didn't find any regression. 👍

mauchede

Nice job @alanpoulain 👍

fzaninotto

Encouraging!

fzaninotto · 2020-01-08T05:45:26Z

src/AdminGuesser.test.js

-            loading: false,
-          },
-        )}
+        {AdminGuesserComponent({


It's strange (and misleading) to call a component function instead of rendering it. See https://kentcdodds.com/blog/dont-call-a-react-function-component for details.

Why don't you do

<Provider store={store}> <AdminGuesserComponent error="..." resources={resources} fetching={false} /> </Provider>

?

I think it has been done this way because the ShallowRenderer from react-test-renderer only allows to render the component "one level deep" and using the Provider is necessary.
See: https://reactjs.org/docs/shallow-renderer.html#overview

You mean in tests? Then don't use shallow rendering, use react-testing-library instead.

The Provider was not needed anymore so we can keep the react-test-renderer.

fzaninotto · 2020-01-08T05:46:13Z

src/AdminGuesser.js

-        <Loading />
-      </TranslationProvider>
-    );
+export const AdminGuesserComponent = ({


"XXXComponent" is a bad name for a component. You don't name your classes "XXXClass", do you?

fzaninotto · 2020-01-08T05:47:05Z

.editorconfig

@@ -0,0 +1,11 @@
+root = true


Is it normal that you commit this file? It feels user based, not project based.

@fzaninotto The whole purpose of this file is to define common options for files' formatting, so everyone working on the project can (with support from IDEs) enforce the same configuration without any manual work. With this you won't get unintentional whitespace diffs. It should be commited to work.

The question is: do project maintainers want this file and current configuration fits project's standards?

It looks good to me (we have it on every other API Platform projects).

fzaninotto · 2020-01-08T05:48:08Z

README.md

+import ReactDOM from 'react-dom';
+import { AdminGuesser, hydra } from '@api-platform/admin';
+
+const Admin = () => <AdminGuesser dataProvider={hydra.dataProvider("https://demo.api-platform.com")} resourceSchemaAnalyzer={hydra.resourceSchemaAnalyzer()} /> // Use your custom data provider or resource schema analyzer


Add line breaks to make it more readable.

Hint: ask prettier to do it for you.

Also, resourceSchemaAnalyzer is long. Why not simply use schemaAnalyzer?

fzaninotto · 2020-01-08T05:48:34Z

README.md

+```javascript
+import React from 'react';
+import ReactDOM from 'react-dom';
+import { AdminGuesser, hydra } from '@api-platform/admin';


The name hydra is very vague IMO. What does this object do? Why don't you export a hydraDataProvider and a hydraResourceSchemaAnalyzer directly?

fzaninotto · 2020-01-08T06:12:52Z

src/hydra/resourceSchemaAnalyzer.js

@@ -0,0 +1,68 @@
+const getReferenceNameField = reference => {


This entire file is hard to read and would greatly benefit from input/output examples in jsDoc.

fzaninotto · 2020-01-08T06:16:05Z

src/Introspecter.js

+import {Loading, useDataProvider} from 'react-admin';
+import ResourceSchemaAnalyzerContext from './ResourceSchemaAnalyzerContext';
+
+const IntrospecterComponent = ({


bad name for a component. By the way, why does this file split its logic in two components that are entirely intricated?

For testing purpose, IIRC.

fzaninotto · 2020-01-08T06:18:47Z

src/Introspecter.js

+
+  useEffect(() => {
+    dataProvider
+      .introspect()


Am I understanding correctly: as Introspecter is called in every Guesser, this will fetch the entire hydra schema once for each resource, field, input? If so, that's a huge waste of bandwidth and a major slowdown. Especially since you've already fetched the resources in AdminGuesser.

The introspection logic is only called once, then the result is stored in a local cache and reused.

admin/src/hydra/dataProvider.js

Line 351 in b022e15

if (apiSchema) return Promise.resolve({data: apiSchema});

fzaninotto · 2020-01-08T06:22:01Z

src/ListGuesser.js


-  render() {
-    const {resourceSchema, fields, ...props} = this.props;
+      setOrderParameters(resolvedOrderParameters);


I don't understand why you setState here even though resolvedOrderParameters might be empty, and you'll have to call the resourceSchema just next line to set it again.

It seems it's because sometimes the order parameters are already in the parsed schema, and sometimes it's not and a supplementary call to the API is needed in order to retrieve them. I'm not sure why but maybe @dunglas or @mauchede can answer?

Actually, Hydra doesn't support (yet) to expose available filters in docs.jsonld, this information is only available in the metadata of the collection, so a call to the 1st page is needed to retrieve these parameters.

fzaninotto · 2020-01-08T06:22:25Z

src/ListGuesser.js

-      </List>
-    );
+  if (!children) {
+    children = fields.map(field => (


don't set a new value for a function argument

alanpoulain · 2020-01-12T19:46:45Z

The Introspecter now uses the useQueryWithStore hook in order to work under optimistic mode.

alanpoulain added 2 commits January 3, 2020 09:56

Update HydraAdmin and AdminGuesser

0037d10

Use hooks for Guessers and Instrospecter

7b215ba

alanpoulain requested review from dunglas, Gregcop1 and jfthuillier January 3, 2020 17:45

Remove AdminGuesser

0670c37

alanpoulain force-pushed the ra3 branch from 0efad43 to 0670c37 Compare January 3, 2020 18:50

alanpoulain mentioned this pull request Jan 3, 2020

feat: first steps to migrate to v3 or react-admin (still in beta) #257

Closed

mleczakm reviewed Jan 3, 2020

View reviewed changes

Some cleanup and use AdminGuesser instead of HydraAdmin

ea9d3ac

alanpoulain force-pushed the ra3 branch from 0d67786 to ea9d3ac Compare January 4, 2020 15:44

Introduce resourceSchemaAnalyzer

c2dd027

alanpoulain force-pushed the ra3 branch from ea23a27 to c2dd027 Compare January 4, 2020 21:25

Add HydraAdmin back

938ea6a

mauchede approved these changes Jan 7, 2020

View reviewed changes

fzaninotto reviewed Jan 8, 2020

View reviewed changes

alanpoulain force-pushed the ra3 branch from acd7fc0 to 2740d7f Compare January 12, 2020 19:43

Fixes

fa6daa7

alanpoulain force-pushed the ra3 branch from 2740d7f to fa6daa7 Compare January 13, 2020 13:48

alanpoulain mentioned this pull request Jan 13, 2020

Fix deprecation warnings about <Admin> props appearing in production marmelab/react-admin#4279

Merged

alanpoulain merged commit ad5ea22 into api-platform:master Jan 13, 2020

alanpoulain deleted the ra3 branch January 13, 2020 14:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

React-admin 3 #263

React-admin 3 #263

alanpoulain commented Jan 3, 2020 •

edited

Loading

alanpoulain commented Jan 3, 2020

mleczakm Jan 3, 2020

alanpoulain Jan 3, 2020

mleczakm Jan 5, 2020

alanpoulain Jan 6, 2020

yudhir commented Jan 5, 2020

Deuchnord commented Jan 6, 2020 •

edited

Loading

mauchede left a comment

fzaninotto left a comment

fzaninotto Jan 8, 2020

alanpoulain Jan 11, 2020 •

edited

Loading

fzaninotto Jan 13, 2020

alanpoulain Jan 13, 2020

fzaninotto Jan 8, 2020

fzaninotto Jan 8, 2020

Wirone Jan 8, 2020

dunglas Jan 10, 2020

fzaninotto Jan 8, 2020

fzaninotto Jan 8, 2020

fzaninotto Jan 8, 2020

fzaninotto Jan 8, 2020

fzaninotto Jan 8, 2020

dunglas Jan 10, 2020

fzaninotto Jan 8, 2020

dunglas Jan 10, 2020 •

edited

Loading

fzaninotto Jan 8, 2020

alanpoulain Jan 11, 2020

dunglas Jan 12, 2020

fzaninotto Jan 8, 2020

alanpoulain commented Jan 12, 2020

		@@ -0,0 +1,68 @@
		const getReferenceNameField = reference => {

React-admin 3 #263

React-admin 3 #263

Conversation

alanpoulain commented Jan 3, 2020 • edited Loading

alanpoulain commented Jan 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yudhir commented Jan 5, 2020

Deuchnord commented Jan 6, 2020 • edited Loading

mauchede left a comment

Choose a reason for hiding this comment

fzaninotto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanpoulain Jan 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas Jan 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanpoulain commented Jan 12, 2020

alanpoulain commented Jan 3, 2020 •

edited

Loading

Deuchnord commented Jan 6, 2020 •

edited

Loading

alanpoulain Jan 11, 2020 •

edited

Loading

dunglas Jan 10, 2020 •

edited

Loading