Upgrade react-fast-pdf and webpack#80160
Conversation
|
|
andriivitiv
force-pushed
the
76303-upgrade-react-fast-pdf-and-webpack
branch
from
955a888 to
d9564d2
Compare
melvin-bot
bot
requested review from
QichenZhu
and removed request for
a team
|
@QichenZhu Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
💡 Codex Reviewhttps://github.com/Expensify/App/blob/d9564d29f72ecb8c6446e373944a3461ab63e0a2/patches/pdfjs-dist/pdfjs-dist+5.4.296+001-fix-worker-was-terminated-error.patch#L20-L24 The patch replaces ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
patches/pdfjs-dist/pdfjs-dist+5.4.296+001-fix-worker-was-terminated-error.patch
Outdated
Show resolved
Hide resolved
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp (N/A. Testing is blocked.)android-native.movAndroid: mWeb Chromeandroid-web.moviOS: HybridAppios-native.moviOS: mWeb Safariios-web.movMacOS: Chrome / Safarimac-web.mov |
|
Testing on Android: HybridApp is blocked because it fails to upload PDFs. android-native-bug.webm |
…act-fast-pdf-and-webpack
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
This has also happened to me. It appears to be resolved after pulling main. |
|
Waiting for the react-fast-pdf PR. |
…act-fast-pdf-and-webpack
to support older browsers
melvin-bot
bot
requested review from
heyjennahay
and removed request for
a team
heyjennahay
left a comment
heyjennahay
left a comment
There was a problem hiding this comment.
Product review not required
| 'victory-native': path.resolve(dirname, '../../node_modules/victory-native/src/index.ts'), | ||
| // Required for @shopify/react-native-skia web support | ||
| 'react-native/Libraries/Image/AssetRegistry': false, | ||
| // Use legacy build of pdfjs-dist to support older browsers |
There was a problem hiding this comment.
Where was this discussed? Which older browsers specificallly are we talking about? We can check production data to see if it matters for us.
There was a problem hiding this comment.
It was discussed here. The new features are supported from Chrome 145, released last month.
There was a problem hiding this comment.
would an alternate solution be to polyfill these browser APIs?
There was a problem hiding this comment.
Yeah it's polyfillable: https://github.com/tc39/proposal-upsert?tab=readme-ov-file#polyfill.
@andriivitiv what do you think?
There was a problem hiding this comment.
It is possible for normal compile-time imports, but not for the worker because it is being bundled as a text file (type: 'asset/source'), which bypasses Babel and its polyfills. And changing this will cause issues while offline
App/config/webpack/webpack.common.ts
Lines 260 to 265 in 59fe031
There was a problem hiding this comment.
@andriivitiv is it possible to put polyfills in src/polyfills instead of using babel?
There was a problem hiding this comment.
claude seems to think we can make the polyfill work without using a legacy version of pdfjs:
diff
diff --git a/Mobile-Expensify b/Mobile-Expensify
index 15c7bec2369..f008b412836 160000
--- a/Mobile-Expensify
+++ b/Mobile-Expensify
@@ -1 +1 @@
-Subproject commit 15c7bec2369402e7e391d56a450fa6b3c97496d2
+Subproject commit f008b412836b7b26e38715cb148bcc34dd11d6df
diff --git a/config/webpack/webpack.common.ts b/config/webpack/webpack.common.ts
index e4c61a56502..00ea07212fd 100644
--- a/config/webpack/webpack.common.ts
+++ b/config/webpack/webpack.common.ts
@@ -246,6 +246,8 @@ const getCommonConfiguration = ({file = '.env', platform = 'web'}: Environment):
{
test: /\.(js|ts)x?$/,
loader: 'babel-loader',
+ // asset/source imports (?raw) must not be transformed so the emitted string matches disk (e.g. pdf worker polyfill)
+ resourceQuery: {not: [/raw/]},
/**
* Exclude node_modules except any packages we need to convert for rendering HTML because they import
@@ -260,7 +262,7 @@ const getCommonConfiguration = ({file = '.env', platform = 'web'}: Environment):
// We are importing this worker as a string by using asset/source otherwise it will default to loading via an HTTPS request later.
// This causes issues if we have gone offline before the pdfjs web worker is set up as we won't be able to load it from the server.
{
- test: new RegExp('node_modules/pdfjs-dist/legacy/build/pdf.worker.min.mjs'),
+ test: new RegExp('node_modules/pdfjs-dist/build/pdf.worker.min.mjs'),
type: 'asset/source',
},
@@ -330,8 +332,8 @@ const getCommonConfiguration = ({file = '.env', platform = 'web'}: Environment):
'victory-native': path.resolve(dirname, '../../node_modules/victory-native/src/index.ts'),
// Required for @shopify/react-native-skia web support
'react-native/Libraries/Image/AssetRegistry': false,
- // Use legacy build of pdfjs-dist to support older browsers
- 'pdfjs-dist$': path.resolve(dirname, '../../node_modules/pdfjs-dist/legacy/build/pdf.mjs'),
+ // Main-thread pdf.js (worker uses standard build + map upsert polyfill; see src/polyfills/mapUpsertPolyfill.js)
+ 'pdfjs-dist$': path.resolve(dirname, '../../node_modules/pdfjs-dist/build/pdf.mjs'),
// Module alias for web
// https://webpack.js.org/configuration/resolve/#resolvealias
'@assets': path.resolve(dirname, '../../assets'),
diff --git a/src/components/PDFThumbnail/index.tsx b/src/components/PDFThumbnail/index.tsx
index 2f41ee9d0ed..bc751089bd1 100644
--- a/src/components/PDFThumbnail/index.tsx
+++ b/src/components/PDFThumbnail/index.tsx
@@ -1,6 +1,8 @@
import 'core-js/proposals/promise-with-resolvers';
+import '@src/polyfills/mapUpsertPolyfill';
+import mapUpsertPolyfillSource from '@src/polyfills/mapUpsertPolyfill?raw';
// eslint-disable-next-line import/extensions
-import pdfWorkerSource from 'pdfjs-dist/legacy/build/pdf.worker.min.mjs';
+import pdfWorkerSource from 'pdfjs-dist/build/pdf.worker.min.mjs';
import React, {useMemo, useState} from 'react';
import {View} from 'react-native';
import {Document, pdfjs, Thumbnail} from 'react-pdf';
@@ -9,7 +11,7 @@ import useThemeStyles from '@hooks/useThemeStyles';
import PDFThumbnailError from './PDFThumbnailError';
import type PDFThumbnailProps from './types';
-pdfjs.GlobalWorkerOptions.workerSrc = URL.createObjectURL(new Blob([pdfWorkerSource], {type: 'text/javascript'}));
+pdfjs.GlobalWorkerOptions.workerSrc = URL.createObjectURL(new Blob([`${mapUpsertPolyfillSource}\n${pdfWorkerSource}`], {type: 'text/javascript'}));
function PDFThumbnail({previewSourceURL, style, enabled = true, onPassword, onLoadError, onLoadSuccess}: PDFThumbnailProps) {
const styles = useThemeStyles();
diff --git a/src/types/global.d.ts b/src/types/global.d.ts
index 76fe292e8a8..4918672ed22 100644
--- a/src/types/global.d.ts
+++ b/src/types/global.d.ts
@@ -27,6 +27,11 @@ declare module '*.lottie' {
export default value;
}
+declare module '*?raw' {
+ const content: string;
+ export default content;
+}
+
// eslint-disable-next-line @typescript-eslint/consistent-type-definitions
interface Window {
setSupportToken: (token: string, email: string, accountID: number) => void;
diff --git a/src/types/modules/pdf.worker.d.ts b/src/types/modules/pdf.worker.d.ts
index a6d70e529b7..c636372b411 100644
--- a/src/types/modules/pdf.worker.d.ts
+++ b/src/types/modules/pdf.worker.d.ts
@@ -1 +1 @@
-declare module 'pdfjs-dist/legacy/build/pdf.worker.min.mjs';
+declare module 'pdfjs-dist/build/pdf.worker.min.mjs';There was a problem hiding this comment.
Updated as suggested.
Tested this on
- Safari 18.6: there was an error in the console. Added
src/polyfills/ReadableStream.jsto fix it. - Chrome 133: PDFs fail to open due to a missing polyfill for Uint8Array.prototype.toHex() (added in Chrome 140). Should we add a polyfill for this, or are versions 139 and older considered too old?
There was a problem hiding this comment.
Let’s add the polyfill. I think generally we want to support major browsers going back 2-3 years
|
We can ignore the failing TS check because it won't continually fail after this PR, and I think it's fine to add JS files for the polyfills |
|
@QichenZhu can you retest please? |
QichenZhu
left a comment
QichenZhu
left a comment
There was a problem hiding this comment.
Just found URL.parse() has only been supported for less than 2 years. There are already three polyfills, and I think it's best not to add more, so I suggest we fall back to the legacy version again.
|
ok, let's revert back to the legacy implementation then. It's pretty interesting that it's so bleeding edge |
|
Reverted |
|
🚧 @roryabraham has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.3.53-0 🚀
Bundle Size Analysis (Sentry): |
|
I reviewed the changes in this PR. It upgrades
No help site changes are required. These are purely internal/technical changes — no user-facing features, settings, UI elements, or workflows were added or modified. |
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.3.53-0 🚀
Bundle Size Analysis (Sentry): |
6 participants
Explanation of Change
react-fast-pdf, which now uses a newer version ofreact-pdfthat fixes a crash when a PDF is replaced before the previous PDF has finished loading:webpackto fix a crash when importingpdf.js:pdf.jsto fixWorker was terminatederror printed in the console:Worker was terminatederror when loading is cancelled mozilla/pdf.js#20503pdf.jsto support older browsers.Fixed Issues
$ #76303
PROPOSAL: #76303 (comment)
Tests
Same as QA Steps.
Offline tests
N/A
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android.native.mov
Android: mWeb Chrome
android.chrome.mov
iOS: Native
ios.native.mov
iOS: mWeb Safari
ios.safari.mov
MacOS: Chrome / Safari
macos.chrome.mov