-
Notifications
You must be signed in to change notification settings - Fork 469
Allow running in node environment #114
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This looks fine, but I would prefer that mutationobserver-shim
is changed rather than putting all of this code on us to maintain separately.
"typings" | ||
], | ||
"dependencies": { | ||
"mutationobserver-shim": "^0.3.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this file now vendored rather than a regular dep? If it needs a change, I'd prefer that dep be updated first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it mutates window directly instead of exporting MutationObserver. It needs to be this way or you need to fork this project and use what I created in this repository (i.e. prevent assigning MutationObserver to window + check if typeof window !== 'undefined'
before using window
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This dep is not maintained and not updated for 2 years
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you really don't want to put it in this repository I suggest forking it and publishing under @kentcdodds/mutationobserver-shim send PR there and use your version for now. I think vendor is OK personally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. How about this, you publish a fork yourself and we can use that 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mutation observer polyfill is the only thing that breaks when you import dtl in a non-DOM environment. It would be great to make it not break to allow modules to consume/extend queries without going through dist/.
@kentcdodds Pushed change that points to my work instead of vendor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome. Thanks!
Would you like to add yourself to the contributors table? |
it's ok, next time |
🎉 This PR is included in version 3.8.3 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Sorry, this busted some stuff and I don't have time to investigate what's going on, so I'm going to revert this commit. |
This reverts commit 1b12385.
Now that jest-dom supports node environment I could add support for it to this package as well.
Fixes #100