👋 App.tsx - #4262
Merged
Merged
👋 App.tsx#4262
App.tsx#4262Conversation
Adds 'webpack-manifest-plugin' for replacing loadables manifest feature
oliverlloyd
requested review from
a team,
JamieB-gu,
iainjchambers-guardian and
marsavar
as code owners
March 16, 2022 10:04
|
Size Change: -93 kB (-6%) ✅ Total Size: 1.48 MB
ℹ️ View Unchanged
|
mxdvl
reviewed
Mar 16, 2022
oliverlloyd
commented
Mar 16, 2022
oliverlloyd
left a comment
Contributor
Author
There was a problem hiding this comment.
I've read through the code and nothing is jumping out to me here. I can't approve because I'm an author but nonetheless I do heartily approve.
Comment on lines
+40
to
+43
| const filename = isDev ? file : manifest[file]; | ||
| const legacyFilename = isDev | ||
| ? file.replace('.js', '.legacy.js') | ||
| : legacyManifest[file]; |
Contributor
Author
There was a problem hiding this comment.
So much clearer! 😻
Comment on lines
-53
to
-56
| Sentry.configureScope((scope) => { | ||
| scope.setTag('edition', editionLongForm); | ||
| scope.setTag('contentType', contentType); | ||
| }); |
Contributor
Author
There was a problem hiding this comment.
Removed in favour of simplicity.
It made sense to include this extra data on errors when it was freely available but now we'd need to refactor the window object to also include these properties which feels excessive
OllysCoding
approved these changes
Mar 16, 2022
OllysCoding
left a comment
Contributor
There was a problem hiding this comment.
So much code removed 😍
Contributor
|
FYI: I stumbled upon a few leftovers of |
Contributor
|
🏝️ #3629 |
JamieB-gu
added a commit
that referenced
this pull request
Jun 2, 2023
Some descriptions have been updated, others deleted: - `content.d.ts` became `content.ts` in #6553, and its functionality has changed over time - `index.d.ts` has different types, and islands have changed - `window-guardian.ts` has different functionality - `ArticleRenderer.tsx` has different functionality - `App.tsx` no longer exists since #4262
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
What does this change?
This PR removes
App.tsxand its associated workings from DCRWhy?
There are two documents which discuss this but in short: The pattern was hard to work with because of shared state and led to more data being sent to readers than was needed
Things of note
Loadable
Is now gone. We used this for lazy loading react components but this is now acheived via the islands pattern
CAPIBrowserTypeIs now gone. This was used type as part of the data set at
window.guardian.appwhich is now deletedwindow.guardian.appIs now gone. We stored data for here to later use as part of hydration. The islands pattern however serialises each islands data directly so there's no need for a centralised object on
windowwebpack-manifest-pluginAdded to replace functionality that was previously provided by Loadable
GA>GADataRenamed to be clearer about what this property is