-
Notifications
You must be signed in to change notification settings - Fork 68
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
Only use legal characters in query config name #197
Conversation
@@ -109,8 +109,7 @@ export default class QueryAggregator { | |||
}); | |||
}); | |||
|
|||
queryConfig.name = | |||
['$$_aggregated', ...Object.keys(queryConfig.queries)].join('-'); | |||
queryConfig.name = 'AggregatedReactRouterQueryConfig'; |
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.
Don't we still need an unique query config name for a given set of queries, because getRelayQueries
uses it as a part of the cache key? Otherwise Relay would probably use the set of queries belonging to the old route after changing the route to a new one (in the case the query params stay the same).
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.
Ugh yes. Hmm.
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 maybe remove $$_
and give other prefix:
Eg.
['react-router-relay', ...Object.keys(queryConfig.queries)].join('-');
Maybe generate query config names sequentially and save them in a map where the key is |
I'm a little bit worried about non-determinism there. It means that the query names will end up being different depending on order of rendering. Wonder if this might lead to odd server/client mismatches. |
You mean isomorphic rendering? E.g. if some app for some reason renders a query config name in a React component? Then yes, it might cause a markup mismatch. The other alternative I can propose is just to encode the name using hex or base62. |
That's what I'm doing with |
I'd prefer to avoid Relay internals, but I'm more concerned about that |
👍 for meaningful query config names, but I would use |
They do have double underscores, because I join with I'd rather not do extra validation on user query names. I feel like it's rather unlikely to come up. I still don't have a repro for the reported issue here, even. |
@taion The latest updates work with both default and custom network layers. |
v0.13.5 |
Fixes #196