-
Notifications
You must be signed in to change notification settings - Fork 143
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
Integrate Server-Side ReactQuery
Support (Non-Breaking Change)
#724
Conversation
packages/pwa-kit-react-sdk/src/ssr/universal/components/route-component/index.js
Show resolved
Hide resolved
appProps, | ||
pageProps, | ||
__STATE_MANAGEMENT_LIBRARY: AppConfig.freeze(res.locals) |
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.
In my mind the AppConfig's freeze/restore API's weren't part of the getProps API. Should we stick this back in the react-rendering
file. I'm guessing you pulled the __STATE_MANAGEMENT_LIBRARY freezing into this file because it looked cleaner no to mention it inside the react-rendering. But it might be a little weird restoring in one file and freezing in another. On top of that, you could end up not freezing if you don't use this HOC, but you still try to restore in the main and rendering files.
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.
Correction: Looks like your getAppConfig handles alway defining the freeze method. I still feel its better off back in the react-rendering file.
@@ -43,6 +43,7 @@ | |||
"@loadable/babel-plugin": "^5.13.2", | |||
"@loadable/server": "^5.15.0", | |||
"@loadable/webpack-plugin": "^5.15.0", | |||
"@tanstack/react-query": "^4.0.10", |
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 doesn't look like we can enforce the version of react-query
that is used in the template code without making a breaking change. That change being making react-query
a peer dependency.
We've used optional peer dependencies in other modules, but optional peer deps aren't supported in some of the lower versions of npm that we support.
Do you think it's ok to just leave this as is, and if someone was to upgrade the version of their react-query
in their project we simply make no promises on it working with the SDK?
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.
Honestly, I'd use the optional peer dep. We've used it before. I think it's just ignored if it's not supported by an NPM version, right?
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.
So for node 7 which doesn't know anything about peerDependenciesMeta optional, if would just think that react-query is a peer dep. Which is a breaking change.
…orceCommerceCloud/pwa-kit into oli-react-query-hackathon
ReactQuery
Support (Non-Breaking Change)
@bendvc For discussion on Monday! Honestly, pretty similar to what you have, just a bit slimmer and dumber!
I think we can do this without any breaking changes at all, if we introduce a function in the SDK that does something like this:
If we did that, we wouldn't need to ask anyone to change a single line of code in their existing apps – not unless they want to opt-in to react-query. The opt-in would be
I haven't checked bundle sizes, etc. on this branch, but I'm pretty sure that react-query won't be included unless you do the opt in there.
**3PP Approval: ** https://gus.lightning.force.com/lightning/r/ADM_Third_Party_Software__c/a0qEE0000005DhNYAU/view