web: Fix extension webpack import not working (close #10981) #19664
Conversation
thy486
force-pushed
the
fix-safari-extension-webpack-require
branch
from
8b7e49f to
0c1291a
Compare
|
For this PR, I packaged it as a safari extension (use my xcode certificate), and this is its running effect. |
| return prop in obj && (obj as Record<PropertyKey, unknown>)[prop] !== ""; | ||
| } | ||
|
|
||
| // Copied from Webpack: https://github.com/webpack/webpack/blob/f1bdec5cc70236083e45b665831d5d79d6485db7/lib/runtime/AutoPublicPathRuntimeModule.js#L75 |
There was a problem hiding this comment.
This link may be confusing.
It actually comes from webpack automatic publicPath. Specifically refers to get publicPath from the currentScript.src.
web/packages/extension/src/ruffle.ts
Outdated
| @@ -57,6 +58,60 @@ function openInNewTab(swf: URL): void { | |||
| window.postMessage(message, "*"); | |||
| } | |||
|
|
|||
| function polyfillCurrentScript(): void { | |||
There was a problem hiding this comment.
I found the relevant code that has used currentScript.src in these files and make polyfills for these files.
- web/packages/extension/src/ruffle.ts
- web/packages/core/src/current-script.ts
web/packages/selfhosted/js/ruffle.js(Intentionally ignored because the extension never use it)
There was a problem hiding this comment.
I find the name confusing. We aren't polyfilling currentScript, but something webpack related?
There was a problem hiding this comment.
Actually can you please add a comment outlining what we are doing here?
There was a problem hiding this comment.
I changed the function name polyfillCurrentScript to polyfillCurrentScriptWebpackPublicPath
There was a problem hiding this comment.
Can you also add a comment as suggested? If possible, I'd suggest it describe both what we are doing and why we need to do it.
web/packages/extension/src/ruffle.ts
Outdated
| ); | ||
| } | ||
| } | ||
| // TODO: Process other situations when mask url not webkit-masked-url://hidden/ |
There was a problem hiding this comment.
Since the current problems are relatively easy to solve in a targeted manner, I made a simple fix. But this may not be the best or most general solution, so I left a todo here
web/packages/extension/src/ruffle.ts
Outdated
| }; | ||
| // Reset webpack public path should be safe when it is using mask url | ||
| if ("webkit-masked-url://hidden/" === scriptUrl) { | ||
| const webpackCurrentPublicPath = __webpack_public_path__; |
There was a problem hiding this comment.
The __webpack_public_path__ runtime code is actually like __webpack_require__.p, so it could be set and get. But this is a webpack only logic.
thy486
force-pushed
the
fix-safari-extension-webpack-require
branch
from
0c1291a to
fca80bc
Compare
There was a problem hiding this comment.
Note: My review comes from the perspective of suggesting simplications to the added code, without actually considering how much of the added code is necessary. My knowledge of webpack is unfortunately limited, so hopefully someone else can review from that perspective.
thy486
force-pushed
the
fix-safari-extension-webpack-require
branch
2 times, most recently
from
e96bdc7 to
9404730
Compare
|
Thank you very much for the review. I have modified my code as required and re-pushed it. Please review it again when you have time |
thy486
force-pushed
the
fix-safari-extension-webpack-require
branch
from
9404730 to
bc02471
Compare
thy486
force-pushed
the
fix-safari-extension-webpack-require
branch
from
bc02471 to
fb16b96
Compare
Close #10981
Continue with PR #19348, this PR is used to fix the
ruffle.jswebpack import module.