[Refactor] Promote new App shell and delete old apps #5651
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Error RangeErrorDangerfile |
Duplicate Sources / Packages - Duplicates found!
|
damassi
force-pushed
the
promote-new-app-shell
branch
from
2899821 to
51f973d
Compare
damassi
commented
May 27, 2020
| @@ -72,7 +72,7 @@ const render = (me: PurchaseAppTestQueryRawResponse["me"], user: User) => | |||
| } | |||
| } | |||
| `, | |||
| wrapper: children => ( | |||
| wrapper: (children) => ( | |||
There was a problem hiding this comment.
Just don't know whats up with prettier these days :/
damassi
force-pushed
the
promote-new-app-shell
branch
2 times, most recently
from
1f12209 to
4b013c5
Compare
damassi
commented
May 27, 2020
| const ReviewRoute = loadable(() => import("./Routes/Review")) | ||
| const AcceptRoute = loadable(() => import("./Routes/Accept")) | ||
| const DeclineRoute = loadable(() => import("./Routes/Reject")) | ||
| const StatusRoute = loadable(() => import("./Routes/Status")) |
There was a problem hiding this comment.
There's diminishing returns by bundle splitting everything. It's fine to split the top most route and then let it load its children, especially if children are really tiny bits of UI as is the case here.
damassi
force-pushed
the
promote-new-app-shell
branch
from
fb609db to
87644e5
Compare
damassi
changed the title
|
Not a comprehensive review by any means but have been clicking around this morning without issue. Looking good — very exciting |
mzikherman
approved these changes
May 27, 2020
| /** | ||
| * ----------------------------------------------------------------------------- | ||
| * | ||
| * Developers! Wait! |
| @@ -25,20 +42,12 @@ app.use(require("./apps/auctions")) | |||
| app.use(require("./apps/auctions2").app) | |||
| app.use(require("./apps/auction_lots")) | |||
|
|
|||
| // TODO: Remove after AB test ends. | |||
| @@ -235,13 +234,7 @@ ArtworkSidebarCommercialContainerState | |||
| ) | |||
| } else { | |||
| const url = `/orders/${orderOrError.order.internalID}` | |||
|
|
|||
| // FIXME: Remove once A/B test completes | |||
There was a problem hiding this comment.
Yes, nice a lot of these places with the special handling for router links.
| @@ -96,7 +96,7 @@ export class Accept extends Component<AcceptProps> { | |||
| logger.error(error) | |||
| switch (error.code) { | |||
| case "capture_failed": { | |||
| const parsedData = get(error, e => JSON.parse(e.data), {}) | |||
| const parsedData = get(error, (e) => JSON.parse(e.data), {}) | |||
There was a problem hiding this comment.
I was commenting on a MP PR of @sweir27 yesterday, is this a new Prettier/setting? Feels a bit verbose, so many paranthesis!
There was a problem hiding this comment.
I agree -- prettier v2's "breaking change" was setting parans to default. That said i can understand the decision, since its always kinda a headache to go back and add adition args or destructure an object or something; at scale this saves a lot of time.
Need to look at our prettier settings and get this stuff consistent / run over repo.
| const ReviewRoute = loadable(() => import("./Routes/Review")) | ||
| const AcceptRoute = loadable(() => import("./Routes/Accept")) | ||
| const DeclineRoute = loadable(() => import("./Routes/Reject")) | ||
| const StatusRoute = loadable(() => import("./Routes/Status")) |
|
Cool! Added deploy block to force and going to merge to get this to staging. |
damassi
changed the title
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ready to review, but do not merge -- need to do some final review-app QA before this goes into master.
This promotes our experimental-app-shell routing framework A/B test to 100% and deletes all of our now-unneeded
apps/app-namecode. Now that this is in I anticipate some rolling cleanup throughout.Review App:
https://new-app-shell.artsy.net/
Details:
apps/artsy-v2contains the new app shell router code. It points atv2/Apps/getAppRouteswhich in turn loads all of our new apps and dynamically splits up JS assets.Next: