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.

Sign up for GitHub

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

Jump to bottom

Add JSDOMNodeJSEnv to ci matrix and fix polyfill #2191

Merged
merged 5 commits into from
Aug 5, 2021
Merged

Add JSDOMNodeJSEnv to ci matrix and fix polyfill #2191

merged 5 commits into from
Aug 5, 2021

Conversation

armanbilge
Copy link
Member

My own attempt to fix the problem reported in #2188.

@armanbilge armanbilge mentioned this pull request Aug 2, 2021
Closed
djspiewak
djspiewak requested changes Aug 2, 2021
View reviewed changes
build.sbt Show resolved Hide resolved
@japgolly
Copy link

japgolly commented Aug 4, 2021

I just tested this and it does indeed work! Thanks for the revision! 👍

@armanbilge
Copy link
Member Author

Hooray!! Thanks for giving it a spin :)

@armanbilge armanbilge mentioned this pull request Aug 4, 2021
Closed
armanbilge
armanbilge commented Aug 5, 2021
View reviewed changes
.gitignore Outdated
Comment on lines 21 to 23
# npm
node_modules/
package-lock.json
Copy link
Member Author

Choose a reason for hiding this comment

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

@vasilmkd @djspiewak Just want to make sure you're aware that I decided to ignore the lockfile (created by the JSDOM dependency), but not sure about this decision at all. @mpilquist, @ChristopherDavenport, @rossabaker, and I have been struggling with this same issue for fs2 and http4s.

In this case its just a test dependency, but @rossabaker makes a good point about repeatable builds. http4s/http4s#4938 (comment)

vasilmkd
vasilmkd requested changes Aug 5, 2021
View reviewed changes
Copy link
Member

@vasilmkd vasilmkd left a comment

Choose a reason for hiding this comment

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

Holding off until a decision about the package-lock file has been made.

@vasilmkd
Copy link
Member

vasilmkd commented Aug 5, 2021

Wait a second, what @djspiewak asked in Discord is actually a very valid question. If JSDOMNodeJSEnv is indeed running in node, which we know it is, then this code path should not be exercised at all, because node supports setImmediate natively. I'm confused here.

@armanbilge
Copy link
Member Author

It's possible they are somehow disabling it, to better emulate the browser?

@vasilmkd
Copy link
Member

vasilmkd commented Aug 5, 2021

I confirm that the native setImmediate works on vanilla node. I'm investigating further.

@armanbilge armanbilge mentioned this pull request Aug 5, 2021
Closed
3 tasks
@armanbilge
Copy link
Member Author

Holding off until a decision about the package-lock file has been made.

We decided to commit this.

@vasilmkd
Copy link
Member

vasilmkd commented Aug 5, 2021

I set up a small node project locally.

npm init
npm install --save jsdom

I copied code from their readme to get a basic set up going:

// From the jsdom readme
const jsdom = require("jsdom");
const { JSDOM } = jsdom;

// these 4 lines are mine
console.log(global.setImmediate);
global.setImmediate(() => console.log('hello setImmediate'));
console.log(setImmediate);
setImmediate(() => console.log('hello again setImmediate'));

// from the jsdom readme
const dom = new JSDOM(`<!DOCTYPE html><p>Hello world</p>`);
console.log(dom.window.document.querySelector("p").textContent); // "Hello world"

And I'm seeing this output:

[Function: setImmediate] {
  [Symbol(nodejs.util.promisify.custom)]: [Getter]
}
[Function: setImmediate] {
  [Symbol(nodejs.util.promisify.custom)]: [Getter]
}
Hello world
hello setImmediate
hello again setImmediate

This IMO means that setImmediate is not being disabled by jsdom.

@armanbilge
Copy link
Member Author

I mean, that all seems reasonable ... now try running it again through the ScalaJS test bridge? https://github.com/scala-js/scala-js-env-jsdom-nodejs

@vasilmkd
Copy link
Member

vasilmkd commented Aug 5, 2021

It fails there, or rather reports undefined. That doesn't mean that jsdom is doing something to disable it. It could just mean that our discovery process is wrong.

@armanbilge
Copy link
Member Author

It could just mean that our discovery process is wrong.

That may very well be true, but I feel very inclined not to touch it. JSDOM is a fake environment, only used for testing. Whatever works should suffice.

@vasilmkd
Copy link
Member

vasilmkd commented Aug 5, 2021

I'm just using the PR to document all discoveries, I think this is more or less good to go.

@armanbilge
Copy link
Member Author

I'm just using the PR to document all discoveries, I think this is more or less good to go.

Oh, absolutely, I appreciate your research efforts! Apologies if came across curt.

@vasilmkd
Copy link
Member

vasilmkd commented Aug 5, 2021

Found it, scalajs-js-env-jsdom-nodejs seems to hide a lot of global things scala-js/scala-js-env-jsdom-nodejs#19.

@armanbilge
Copy link
Member Author

Cool, that's what I thought!

@djspiewak djspiewak merged commit 62d7d63 into typelevel:series/3.x Aug 5, 2021
@armanbilge armanbilge mentioned this pull request Aug 23, 2021
Merged
9 tasks
@armanbilge armanbilge mentioned this pull request Oct 1, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@japgolly japgolly japgolly left review comments

@vasilmkd vasilmkd vasilmkd approved these changes

@djspiewak djspiewak djspiewak approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@armanbilge @japgolly @vasilmkd @djspiewak