Skip to content
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

Fix the polyfill for JSDOM #9

Closed
djspiewak opened this issue Sep 6, 2021 · 9 comments · Fixed by #17
Closed

Fix the polyfill for JSDOM #9

djspiewak opened this issue Sep 6, 2021 · 9 comments · Fixed by #17

Comments

@djspiewak
Copy link
Collaborator

Yeah it actually just doesn't work:

[info] org.scalajs.macrotaskexecutor.MacrotaskExecutorSuite:
[info]   + sequence a series of 10,000 recursive executions without clamping 25.78s
[info]   + preserve fairness with setTimeout 0.11s
[info]   + execute a bunch of stuff in 'parallel' and ensure it all runs 13.14s

"without clamping"

If we can fix this, we can uncomment the assertion which ensures that we're actually not being clamped.

@armanbilge
Copy link
Member

Update on this: I tried 2 things.

  1. Fixing the postMessage detection stuff, since postMessage does appear to be implemented in JSOM. This gets us:
/workspace/scala-js-macrotask-executor/node_modules/jsdom/lib/jsdom/living/helpers/events.js:15
const event = createAnEvent(e, target._globalObject, eventInterface, attributes);
                                      ^

TypeError: Cannot read property '_globalObject' of undefined
  at fireAnEvent (/workspace/scala-js-macrotask-executor/node_modules/jsdom/lib/jsdom/living/helpers/events.js:15:41)
  at Timeout._onTimeout (/workspace/scala-js-macrotask-executor/node_modules/jsdom/lib/jsdom/living/post-message.js:36:7)
  1. Implementing the IE polyfill which relies on <script>. This doesn't work because of JSDOM security measures:

    The jsdom sandbox is not foolproof, and code running inside the DOM's <script>s can, if it tries hard enough, get access to the Node.js environment, and thus to your machine. As such, the ability to execute scripts embedded in the HTML is disabled by default

    https://github.com/jsdom/jsdom#executing-scripts

@armanbilge
Copy link
Member

armanbilge commented Sep 7, 2021

Update on 2: it seems the JSDOM env for Scala.js already disables that security measure. So, my implementation of the polyfill is broken then or something else weird is going on.

@armanbilge
Copy link
Member

Also worth nothing that the postMessage implementation in JSDOM itself uses setTimeout:
https://github.com/jsdom/jsdom/blob/master/lib/jsdom/living/post-message.js#L35-L37

@djspiewak
Copy link
Collaborator Author

Also worth nothing that the postMessage implementation in JSDOM itself uses setTimeout:

Wtf… Seriously?

@djspiewak
Copy link
Collaborator Author

How is it that we don't have access to setImmediate in jsdom? It's a node environment, and they seem to use it internally: https://github.com/jsdom/jsdom/blob/9dae17bf0ad09042cfccd82e6a9d06d3a615d9f4/lib/jsdom/living/xhr/xhr-utils.js#L410

@armanbilge
Copy link
Member

How is it that we don't have access to setImmediate in jsdom

IDK, they do some sandboxing to make the global scope behave like a browser scope.

However, the README alludes to someway to access Node:

The jsdom sandbox is not foolproof, and code running inside the DOM's <script>s can, if it tries hard enough, get access to the Node.js environment, and thus to your machine.

I'm just not clever enough to see how.

@djspiewak
Copy link
Collaborator Author

if it tries hard enough

image

JK. Maybe we should just give up on jsdom. The polyfill is functional on that platform, it's just slow.

@armanbilge
Copy link
Member

🙃 Let's keep this issue and maybe mark it help wanted? Who knows, inspiration may strike.

In the meantime, I can disable the clamping test for only JSDOM so we can run it in the other environments.

@djspiewak
Copy link
Collaborator Author

In the meantime, I can disable the clamping test for only JSDOM so we can run it in the other environments.

I think that's the right approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants