Skip to content

web: Fix extension webpack import not working (close #10981) #19664

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

Closed

Conversation

thy486
Copy link
Contributor

@thy486 thy486 commented Mar 1, 2025

Close #10981
Continue with PR #19348, this PR is used to fix the ruffle.js webpack import module.

@thy486 thy486 force-pushed the fix-safari-extension-webpack-require branch from 8b7e49f to 0c1291a Compare March 1, 2025 12:18
@thy486
Copy link
Contributor Author

thy486 commented Mar 1, 2025

For this PR, I packaged it as a safari extension (use my xcode certificate), and this is its running effect.
Snapshot 2025-03-01 20 26 30

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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This link may be confusing.
It actually comes from webpack automatic publicPath. Specifically refers to get publicPath from the currentScript.src.

);
}
}
// TODO: Process other situations when mask url not webkit-masked-url://hidden/
Copy link
Contributor Author

@thy486 thy486 Mar 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

};
// Reset webpack public path should be safe when it is using mask url
if ("webkit-masked-url://hidden/" === scriptUrl) {
const webpackCurrentPublicPath = __webpack_public_path__;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Related documents

@kjarosh kjarosh added A-web Area: Web & Extensions extension Related to the Ruffle WebExtension T-fix Type: Bug fix (in something that's supposed to work already) safari labels Mar 2, 2025
@thy486 thy486 force-pushed the fix-safari-extension-webpack-require branch from 0c1291a to fca80bc Compare March 4, 2025 06:07
@danielhjacobs danielhjacobs requested a review from evilpie April 7, 2025 15:37
Copy link
Contributor

@danielhjacobs danielhjacobs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 thy486 force-pushed the fix-safari-extension-webpack-require branch 2 times, most recently from e96bdc7 to 9404730 Compare April 8, 2025 18:54
@thy486
Copy link
Contributor Author

thy486 commented Apr 8, 2025

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 thy486 requested a review from danielhjacobs April 9, 2025 01:45
@thy486 thy486 force-pushed the fix-safari-extension-webpack-require branch 3 times, most recently from fb16b96 to 949eaf2 Compare April 18, 2025 00:44
@danielhjacobs
Copy link
Contributor

danielhjacobs commented Apr 18, 2025

Let me see if I understand the logic:

When we do script.src = url, Safari masks that to "webkit-masked-url://hidden/".

The previous change attempted a targeted fix: the only reason we need to know script.src is because the url has a query parameter called ID that needs to be used in the postMessage. You worked around that by setting an attribute on the script called ruffle-id and using that.

And we need this change because:

webpack's code for the automatic public path also uses the script.src. That src is "webkit-masked-url://hidden/" so that's what webpack tries to load.

So the new code says:

If script.src = url and then immediately script.src !== url, ruffle-src-polyfill = url. Safari doesn't mask that because it's not a script.src.

If ruffle-src-polyfill and __webpack_public_path__ === scriptAutoPublicPath, __webpack_public_path__ is updated to the original ruffle-src-polyfill converted to a public path (https://github.com/webpack/webpack/blob/f1bdec5cc70236083e45b665831d5d79d6485db7/lib/runtime/AutoPublicPathRuntimeModule.js#L75).

If script.src === "webkit-masked-url://hidden/", getScriptOriginalSrc gets ruffle-src-polyfill, otherwise it gets script.src. That is used to determine if isExtension instead of always the script.src as before.

@danielhjacobs
Copy link
Contributor

I hate to do this after you put so much work into this PR, but I wanted to improve the code logic and ended up superseding this PR. Can you test #20255? Ideally, it will fix the issue just as well.

@danielhjacobs
Copy link
Contributor

Please test today's nightly with Safari. If it works, this PR can be closed.

@thy486
Copy link
Contributor Author

thy486 commented Apr 27, 2025

Please test today's nightly with Safari. If it works, this PR can be closed.

It works. Thank you for your work.

@danielhjacobs
Copy link
Contributor

Superseded by #20264

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-web Area: Web & Extensions extension Related to the Ruffle WebExtension newsworthy safari T-fix Type: Bug fix (in something that's supposed to work already)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extension does not work in Safari 16
5 participants