Build and load annotator bundle as a non-module script #4353
Conversation
|
This change may annoyingly get in the way of lazily loading annotator functionality via dynamic import. I need to test whether that is affected by this issue or not. |
Safari does not support loading module scripts in XHTML pages that are served with the "application/xhtml" mime type [1]. This is very rare on the open web (XHTML is rare and XHTML documents served with "application/xhtml" as opposed to "text/html" as the MIME type are rarer still [2]) but more common in ebooks. This partly fixes #4350. [1] https://bugs.webkit.org/show_bug.cgi?id=227469 [2] https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/XHTML#xhtml_document
As noted in [1], much (most?) XHTML content on the web is actually served with the "text/html" mime type, including the existing documens in the dev server which declare themselves to be XHTML. However serving an XHTML document with the correct "application/xhtml+xml" mime type alters various browser behavior, so we need a page that tests this. [1] https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/XHTML#xhtml_document
robertknight
force-pushed
the
load-annotator-bundle-as-classic-script
branch
from
233f348 to
a898c4c
Compare
|
I added a test page at http://localhost:3000/document/xhtml-test. Visit this page in Safari, Firefox and Chrome and verify that the client loads and the adder appears when text is selected. |
Codecov Report
@@ Coverage Diff @@
## master #4353 +/- ##
==========================================
- Coverage 99.17% 99.16% -0.02%
==========================================
Files 225 225
Lines 8588 8596 +8
Branches 2030 2028 -2
==========================================
+ Hits 8517 8524 +7
- Misses 71 72 +1
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
| bundleConfig({ name: 'annotator', entry: 'src/annotator/index.js' }), | ||
| bundleConfig({ | ||
| name: 'annotator', | ||
| entry: 'src/annotator/index.js', |
There was a problem hiding this comment.
OK, this makes sense. Only the annotator bundle needs to be iife as we have control over the sidebar frame's MIME- and document-types.
| * in case it has been loaded in the same document previously. | ||
| * @param {object} options | ||
| * @param {boolean} [options.esModule] - Whether to load the script as an ES module | ||
| * @param {boolean} [options.forceReload] - Whether to force re-evaluation of an ES module script |
There was a problem hiding this comment.
I'm not seeing this option actually used (i.e. set to true) anywhere anymore? Is it still relevant?
There was a problem hiding this comment.
forceReload is not actually used anywhere currently. I was on the fence about removing it. On the one hand it is not currently used. On the other it is a useful reminder of how to force a module script reload if needed.
Safari does not support loading module scripts in XHTML pages that are served
with the "application/xhtml" mime type [1]. This is very rare on the open web (XHTML
is rare and XHTML documents served with "application/xhtml" as opposed to
"text/html" as the MIME type are rarer still [2]) but more common in ebooks.
There is open issue for Chrome that says it has the same issue, yet in practice it seems that the client does load properly in Chrome and Firefox on an XHTML page. See #4350 (comment) for some test pages.
[1] https://bugs.webkit.org/show_bug.cgi?id=227469
[2] https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/XHTML#xhtml_document
TODO: