-
Notifications
You must be signed in to change notification settings - Fork 10k
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
Break import cycles, in the viewer, for PDFViewerApplication
#17646
Break import cycles, in the viewer, for PDFViewerApplication
#17646
Conversation
This is very old code, which can (ever so slightly) be simplified now that import maps are available.
With the changes in PR 17588 we're already importing the relevant code via the `web/app.js` file.
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.241.84.105:8877/dd6cb06c6d90fa5/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/dd6cb06c6d90fa5/output.txt Total script time: 1.21 mins Published |
/botio test |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.193.163.58:8877/b665d8969a43ca2/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.241.84.105:8877/d72b2ad9281884e/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/d72b2ad9281884e/output.txt Total script time: 24.89 mins
Image differences available at: http://54.241.84.105:8877/d72b2ad9281884e/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/b665d8969a43ca2/output.txt Total script time: 39.86 mins
Image differences available at: http://54.193.163.58:8877/b665d8969a43ca2/reftest-analyzer.html#web=eq.log |
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.
Looks good to me, with one question. Thanks!
Currently the `web/app.js` file pulls in various build-specific dependencies, via the use of import maps, and those files in turn import from `web/app.js` thus creating undesirable import cycles. To avoid this we instead pass in a `PDFViewerApplication`-reference, immediately after it's been created, to the relevant code. Note that we use an ESLint plugin rule, see `import/no-cycle`, that is normally able to catch import cycles. However, in this case import maps are involved which is why this wasn't caught.
9ec439a
to
e98b9b0
Compare
Currently the
web/app.js
file pulls in various build-specific dependencies, via the use of import maps, and those files in turn import fromweb/app.js
thus creating undesirable import cycles.To avoid this we instead pass in a
PDFViewerApplication
-reference, immediately after it's been created, to the relevant code.Note that we use an ESLint plugin rule, see
import/no-cycle
, that is normally able to catch import cycles. However, in this case import maps are involved which is why this wasn't caught.