feature: Enabled Remaining Routes in Novo #6784
Conversation
| @@ -18,7 +18,7 @@ const ArtistsByLetterRoute = loadable( | |||
| } | |||
| ) | |||
|
|
|||
| export const artistsRoutes: RouteConfig[] = [ | |||
| export const routes: RouteConfig[] = [ | |||
There was a problem hiding this comment.
Why are we renaming these, if we need to import as artistsRoutes anyways?
There was a problem hiding this comment.
It felt like a reasonable time to make the names consistent. There are potential benefits of having a consistent interface if we wanted to make the route loader smarter in the future.
I will revert them if you'd like.
There was a problem hiding this comment.
routes seemed to be the preference.
There was a problem hiding this comment.
Not having them named artistsRoutes, artworkRoutes, etc, is actually tech debt that we've been needing to fix for some time because its causing us problems differentiating between different relay queries (due to the need for relay output fragments to include the file name). We've just never gotten around to fixing this. For example, this came up yesterday:
https://artsy.slack.com/archives/CP9P4KR35/p1607534922411300
[RelayCache#set] Success {"queryId":"routes_ShowInfoQuery","variables":{"slug": slug}}
If we had this route file named showRoutes then there would have been no confusion here in our server logs during the incident.
There was a problem hiding this comment.
Those query names are independent of the named export. As far as the export if concerned routes is good and appropriate from a systems architecture perspective, plus a stack trace will provide the correct data.
💯 agree that those queries should be updated, we have similar issues with anonymous functions throughout force. Taking a look at the exports for the files I modified they suffer from the same inconsistency so let's not block changes for unrelated reasons:
https://github.com/artsy/force/blob/master/src/v2/Apps/Conversation/routes.tsx#L16
There was a problem hiding this comment.
The thing is is that relay, from a compiler level, requires that the file name be prefixed in the query; we can't update the query name without updating the filename.
Example error:
ERROR:
Parse error: Error: RelayFindGraphQLTags: Operation names in graphql tags must be prefixed with the module name and end in "Mutation", "Query", or "Subscription". Got `conversationRoutes_ConversationQuery` in module `routes`. in "Apps/Conversation/routes.tsx"(This change can happen in a different PR tho, just a bit of FYI background)
There was a problem hiding this comment.
Sounds like everything is good as is then, I don't plan on renaming the files.
This change adds the remaining routes to Novo and in doing so adds their contents to novo assets. This is needed so that we can begin to analyze what the contents and potentially reduce its size before swapping over a live page.
icirellik
force-pushed
the
feature-enable-routes-in-novo
branch
from
2f1649a
to
28a5906
Compare
This change adds the remaining routes to Novo and in doing so adds their
contents to novo assets. This is needed so that we can begin to analyze
the contents and potentially reduce its size before swapping over
a live page.