Improve Web Worker compatibility (partial)#242
Conversation
|
Thank you very much @wSedlacek for such a thoughtful analysis of solutions and choosing the best one for your PR! |
|
@wSedlacek I'm having hard times importing isomorphic-dompurify from inside of a Web Worker script. I don't have much experience with web workers. Could you please instruct me how to do it? I'm using Vite vanilla template and importing this way In my package.json: |
|
I’ll create an example today for testing and get back to you. |
|
So Update: import createDOMPurify from "isomorphic-dompurify";
import { parseHTML } from "linkedom";
const { window } = parseHTML("<!DOCTYPE html><body>");
const DOMPurify = createDOMPurify(window);
self.onmessage = (event) => {
const purified = DOMPurify.sanitize(event.data);
console.log(`On Worker Thread: ${purified}`); // Should be <img src="x”> but is <img src=x onerror=alert(1)//>
};The only issue is that while it does run, it doesn’t actually santaize anything at all. |
|
Thank you @wSedlacek for your work on this issue! Really great job! However, I think we cannot release this change without |
|
@coderabbitai wdyt? |
|
Tip For best results, initiate chat on the files or code changes. @kkomelin, thank you for asking my opinion! This is an interesting technical decision. Here's my analysis: Arguments for merging this PR:
Arguments against merging:
My recommendation: I'd lean toward merging the PR with clear documentation caveats, because:
However, critical documentation updates are needed:
This way, you get the benefits of better code without misleading users about capabilities. What do you think? |
|
@coderabbitai Thanks for your input. Would you recommend releasing this change as a major version? |
|
Tip For best results, initiate chat on the files or code changes. @kkomelin, based on semantic versioning principles, I would recommend this as a minor or patch version release, not a major version. Here's why: Why not a major version:
Version recommendation: I'd suggest a minor version (0.X.0) bump because:
Alternatively, a patch version (0.0.X) could be justified if you view this purely as fixing a bug (the crash on import). Important considerations:
Major versions should be reserved for:
This change doesn't meet any of those criteria, so a major version bump would likely confuse users who expect breaking changes in major releases. Would you like me to help draft changelog or documentation updates for this release? |
Current Behavior
When importing into a worker environment the following runtime error occurs
Change in behavior
The runtime error does not occur and
DOMPurifyis accessible both as a exportand on
self.DOMPurify/globalThis.DOMPurifyin workersDeveloper Notes
There are probably a dozen different ways to fix this issue, the one chosen was
picked for being the one with the fewest number of changes while being
backwards compatible all the way back to browser versions from 2012 (Firefox
back to 2004)
See: https://developer.mozilla.org/en-US/docs/Web/API/Window/self#examples
Other approaches considered
typeof window !== "unefined"windowexists and if this code isexecuting in a worker was considered however after this check was done the
split in exuection after this was detected would have either needed to use
selfor pollyfilwindowonself. Sinceselfalways works it seemedmore straight forward to just always use it instead of
windowExample:
globalThisbrowser however it is a much newer feature and not available on all platforms
(ie, React Native, etc.)
Example:
typeof window !== "undefined"but not assigning toself.DOMPurifywindowin worker environmentseffectively killing the polyfil nature of this in worker environments but
still allowing the import
Example:
DOMPurifyaccessible onself/globalThisas the previous method however it would rely on a featureonly aviable in newer JavaScript environments.
Example:
fixes: #240