-
Notifications
You must be signed in to change notification settings - Fork 153
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
Introduce Magento Admin FE Guidelines #231
Conversation
75dd3ec
to
38e9b05
Compare
adb767d
to
8f7fe9c
Compare
8f7fe9c
to
eea1627
Compare
"@reach/router": "^1", | ||
``` | ||
|
||
#### State management |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point I exclusively use React's context
. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you can swing it, that's fantastic! It's still a bit of a new concept, however, so I don't want to take away Redux just yet. I'd be interested in reviewing some code you have to see what the pros & cons are at this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds like a frontend guild talk idea ^^
"@queso/kebab-case": "^1", | ||
``` | ||
|
||
#### SSR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Next.js recommends the use of isomorphic-unfetch for universal data fetching. Might be another option to consider as it works very similar to the built in fetch
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. I've been down this 🐇 🕳 a few times now and I keep coming to the same conclusion. Take a look at ky's usage compared to plain fetch. Much more readable and expressive and comes with 1st-class TypeScript support to boot! Happy to open a conversation on the matter though if you have some specific arguments to present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ky's API looks nice. I haven't used it in a project before, but I'd be willing to try that one. I'm not sure how I feel about it automatically throwing an error on any code that's not in the 200s, but I think I could get used to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed that KY only lists Chrome, Safari, and Firefox as supported browsers. A data fetching solution that doesn't support Edge or IE probably isn't going to work. Magento docs say we support back to IE11
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, this is intentional. Sindre does this with all of his most recent libraries, directing users to polyfill as appropriate. It doesn't mean it won't work with IE11/Edge. Refer to react-app-pollyfill.
"@jedmao/redux-mock-store": "^2", | ||
``` | ||
|
||
#### Testing library |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call here 👍🏼
Customize your CRA configuration with [craco][]. | ||
|
||
- [Don't eject](https://create-react-app.dev/docs/alternatives-to-ejecting). | ||
- Avoid forking, because you can oftentimes do the same thing by creating your own [craco][] plugin. See [craco plugins on npm](https://www.npmjs.com/search?q=craco%20plugin) and the [craco recipes](https://github.com/sharegate/craco/tree/master/recipes) for guidance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be interesting to see if we can get everything from the react-scripts fork working in Craco. If it's possible, this approach works for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's exactly what I'm challenging you to do here. I'm actually going to create a craco plugin for Linaria today if I have enough time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Challenge accepted. I'll see if I can get some time on the board on in our grooming meeting Monday to work on this.
"@queso/kebab-case": "^1", | ||
``` | ||
|
||
#### SSR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ky's API looks nice. I haven't used it in a project before, but I'd be willing to try that one. I'm not sure how I feel about it automatically throwing an error on any code that's not in the 200s, but I think I could get used to it.
"@reach/router": "^1", | ||
``` | ||
|
||
#### State management |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds like a frontend guild talk idea ^^
|
||
## Problem | ||
|
||
There are already a handful of React applications built for Magento Admin use, but they have different dependencies and versions between them and we don't yet have a micro front-end architecture to manage them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jedmao does it mean that upgrade of Magento Admin frontend (currently, UI component-based) is somewhere on the horizon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are no plans for that as of yet.
See [The Future of React Router and @reach/router](https://reacttraining.com/blog/reach-react-router-future/). Basically, the new API will be much closer to `@reach/router` than React Router, so this prepares us for that future. | ||
|
||
```json | ||
"@reach/router": "^1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you found a way you like to make @reach/router support hash routing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. Is that an important feature for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so. We'll need client-side routing within the React app that doesn't interfere with Magento's built-in server-side routing. I would use a hash router for that. Did you have a different approach in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Short answer is no, hash routing is not supported and seeing as react-router is going to merge with reach router, I guess we'll have to see if it survives the merge. Take a look at this thread that suggests using a memory router; though, also points out the fact that it won't survive a page refresh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two big reasons to use reach router:
- The API is most compatible with the future merge of react router.
- Nested routes are way nicer.
That said, if you really can't make it work with reach router, I guess you'll have to use react router (for now), understanding that the API will change significantly and you might even lose your hash routing capabilities, preventing you from upgrading it.
No great answer here. It's a double edged ⚔️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'd read that thread when I was trying to figure out how to solve this issue. I think the value of having the hash router now is worth a slightly more involved migration later on to my team.
Problem
There are already a handful of React applications built for Magento Admin use, but they have different dependencies and versions between them and we don't yet have a micro front-end architecture to manage them.
Solution (for now)
We must do our best to align common dependencies manually.