-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
add support for MutationObserver #639
Comments
I also need this. I would be willing to work on a pull request for this if someone can point me in the right direction. |
@SegFaultx64 The URL provided by schettino72 in the first post on this issue is the "right direction". |
Fair enough, I am not even sure how we would register the observers in a project this large. Is there a place where stuff like that is getting stored? |
@SegFaultx64 It is unclear to me what the best method would be because I've not looked at that part of jsdom. When I worked on fixing the NS methods (see #727; nominally, it is a bug fix but it required substantial changes to the guts of jsdom) I looked for analogical structures and went with that for deciding where to add new classes/data/etc. I pretty much went ahead with what I thought was best, Domenic made some comments, I adjusted what needed adjustment, and that was that. |
@lddubeau Okay, thanks for the pointers. I actually found a nice looking shim for MutationObserver https://github.com/megawac/MutationObserver.js/blob/master/MutationObserver.js. It looks like a nice jumping off point. I will get started on this later today. |
My only advice would be to peruse https://dom.spec.whatwg.org/ and look into the details of where a mutation record is enqueued. Finding the appropriate counterparts in jsdom might be tricky since we're still transitioning to the new DOM standard, but at least the spec will give you guidance on what needs to be implemented. |
any updates on this? |
@kresli pull request welcome! |
I have no idea how to do it, but I've decided to do some experiments. Worst case I learn a little, best case I might be able to contribute. I'm currently trying to understand how jsdom is structured and how webidl is working (and is used in jsdom) ~~Would it be possible to depart from this webidl on MutationObserver: https://dxr.mozilla.org/mozilla-central/source/dom/webidl/MutationObserver.webidl~~~ I understand it a bit better now - the interface(s) to use should be those described here: https://dom.spec.whatwg.org/#interface-mutationobserver - right? |
@henrikkorsgaard correct! To get it working in jsdom, you need to start with a couple things:
You then "just" need to implement the observe, disconnect, and takeRecords methods, plus the constructor, in the impl class. In general you should try to follow the spec as much as possible. (@Sebmaster, can you explain how to do the constructor?) In this case, unlike something simple like CharacterData, a lot of your changes will require modifying other jsdom impl files. For example, the spec says "Each node has an associated list of registered observers." This will require adding an instance variable to the Node impl. The spec also says "Each unit of related similar-origin browsing contexts has a mutation observer compound microtask queued flag, which is initially unset, and an associated list of MutationObserver objects, which is initially empty." This is a little trickier, since "unit of related similar-origin browsing contexts" is not obvious in jsdom. What I would do for this is to start by just putting them on Window objects. Window objects do not use IDL yet so you'll just add an underscore-prefixed property in Window.js. In the future we can try to worry about making sure things are tracked across multiple windows (i.e. take care of iframes). But for now window is a good place to put them. Finally, a lot of the implementation will be taken care of by finding appropriate places to "queue a mutation record". If you go to https://dom.spec.whatwg.org/#queue-a-mutation-record and click "queue a mutation record" you can see all the places in the spec that happens. This will be a bit tricky because jsdom doesn't have all the exact same hooks the spec does for when things happen. But it should be doable, using jsdom's _attrModified, _descendantRemoved, and _descendantAdded hooks. https://github.com/tmpvar/jsdom/blob/master/lib/jsdom/living/attributes.js also has "TODO mutation observer stuff" throughout. Hope that helps! |
Great :) Again, I'm not super experienced with webidl and larger codebases. I'm also a bit swarmed with a few deadlines ;) I will do my best and give it a go over the next few weeks. I will also try and make a test case or two. |
Ok, I've gotten so far to create the idl and Impl and required it into window.js. Now I am trying to create a test file for it and I can call new MutationObserver() with the window prefix.
Am I missing some trickery here or is there somewhere I need to expose the MutationObserver as a global object (called without calling it on the window object). I've added the following line in Window.js
Sorry about all this asking. I'm just trying to establish some entry-points so I can start testing and follow the jsdom architecture/pattern. |
Ah, sorry, I forgot that part. You'll need to add a line like https://github.com/tmpvar/jsdom/blob/28f00b30236d540df1777ca6c2c0ee9e5e19fe5b/lib/jsdom/living/index.js#L28 |
I have a question related to queueing the mutation task. The spec seem to indicate a central place where multiple micro tasks are queued and excuted in order. I can find such place in jsdom and I think @domenic's idea on having that responsibility in Window.js will work (webkit adds mutation records to a dedicated thread/queue). But this would also require some form of queued and more importantly some logic that executes the what is in the queue. This leads me to two questions: Is there any other components in jsdom that might benefit from such a queue? Is there something I should consider here in terms of architecture and (future) integration? Would it be smartest to execute tasks in the queue based on a timer (is there a global tick in jsdom?) or just based on a promises/done callback structure? I'm focusing on getting the very basic implementation done and then writing extensive test cases to guide the future implementation. |
So, a microtask is basically just So: when the spec says "Queue a compound microtask to notify mutation observers", you do |
For tests, be sure to check out https://github.com/tmpvar/jsdom/blob/master/Contributing.md. There may be some existing web-platform-tests you can use (although those aren't always that easy to get passing for a from-scratch implementation). And any tests you author should be in the to-upstream folder, following that format. |
This might not be the right place to ask, but I'm having some trouble understanding how to pass webidl scheme objects (e.g. /generated/MutationRecord.js) back and forth between the impl files (Node-impl -> MutationObserver-Impl). I'm tempted to construct the objects as pure JSON objects that mirror the schema of MutationRecords and just pass that from Node-impl to MutationObserver when a mutation occurs. I guess this would break the ambition to implement all aspects of MutationObservers as described in the specification. I suspect this is because I am not familiar with jsdom webidl architecture/object model/interfaces. An example within the code would also help me understand working with /generated/ objects in the impl files. |
We should probably document this somewhere, but here goes: In general, any arguments going through the generated API will automatically be unboxed/reboxed, so you never have to care about the generated objects - only the implementation objects are important. Except - not really. This would be the case, if we had everything switched to IDL-based already, which is not the case. Arguments and return values do work, however every time you interact with a non-IDL class (like Window), you'll have to do the unboxing/reboxing when you provide them any arguments yourself (see for example https://github.com/tmpvar/jsdom/blob/master/lib/jsdom/living/events/EventTarget-impl.js#L103, where we have to unwrap a Window, since Window is not idl'd yet, but EventTarget (of which Window inherits from) is). You do this manual boxing with So in general, you want to construct an Impl object. To do so, you require the generated file and call I realize that this is not nearly in-depth enough to grasp the whole thing (I think), so if you have something more specific I'd be happy to explain more. |
Thank you @Sebmaster, it helped a lot. Yes, documenting this with a few examples on how one should integrate and use API/objects would be helpful, but I think I know enough for the next few steps. |
Quick update! I will update them locally for my purpose but I'm not sure if I will commit these to W3C/jsdom. That is, unless I have the time to finish that task as well. But attributtes mutations now pass most of the tests and I will make a pull request sometime during the weekend or in the beginning of next week. |
That's super-exciting! Can you explain in a bit more detail what the issue is? I wasn't quite able to follow web-platform-tests/wpt#2482 so maybe just: what property value of a MutationRecord do the tests expect, and what do you think the spec requires? |
The issue is that the test does not specify a full mutationRecord to compare with. So in the first test case the test will change the id value and as a consequence the returned mutation record will have the old value property. The oldValue property is not defined in the expected object and the test will fail. As I understand it a mutation record should always contain all the properties, even when they are null. In that case they should be DOMString null. When properties are not set in the testcase, the test will end up comparing null (typeof object) with null (typeof string) and fail. The tests are constructed lazily, e.g. undefined fields are automatically set to null (object). This will cause tests to fail when a) the mutationRecord returns a property that is not set in the expected record object (e.g. oldValue) and b) when the mutation record property is DOMstring null (e.g. attributteNamespace). Makes sense? If I'm right is is fairly easy to fix, but requires going through all the cases one by one. This can be a bit hard as an outsider to both jsdom and w3c testing :) |
I'm sorry, can you simplify? I'd prefer an answer in the form: the tests expect oldValue to be null or undefined, but the spec gives a value "null". Or similar. |
The test referenced above implicitly expect oldValue to be null. It should be "n" All the test cases in that file expects attributeNamespace to be null (object), they should be DOMString null according to the specification. |
The type of attributeNamespace is DOMString?, so it's allowed to be null (not "null", just null). Thanks for clarifying on the oldValue case :). |
Ok, thanks for clarifying - I think I've missed important aspects of the spec. |
I've added a branch MutationObserver in my fork and made a push to it. Currently, Attribute and CharacterData mutations are passing the most important w3c tests. Where do I document the tests that do not pass for known reasons/issues (missing Range support #317 etc.)? |
What we do is add them to the index.js file for web-platform tests, but commented out, with a comment as to why. See e.g. what we do for template. |
* feat(waitForElements): add implementation, tests, docs Add `mutationobserver-shim` to `devDependencies` to provide `MutationObserver` in jest tests where `jsdom` has no built-in support for it: jsdom/jsdom#639 * docs(contributors): update sompylasar * CR changes - rename `waitForElements` to `waitForElement` - move `mutationobserver-shim` to dependencies, import from `waitForElement` to provide the polyfill to the users of `dom-testing-library` - fix `kcd-scripts` version to match `master` branch - add synchronous `callback` call to detect an element if it's already present before any DOM mutation happens - add/change tests about synchronous `callback` call - tweak variable names in docs examples - add docs about the default `container` option value - add docs example about querying multiple elements - add docs about the `mutationobserver-shim` polyfill - add docs link and anchor to `mutationObserverOptions` - add docs link to MDN from the second mention of `MutationObserver` * fix(waitForElement): ensure it works with default callback Should wait for the next DOM change, as advertised in the docs. The default value is `undefined` so that the `options` object can be used while still keeping the default callback: ``` waitForElement(undefined, {attributes: true}) ``` * CR: tweak docs examples for wait and waitForElement - use `container` in the examples as this is a more popular use case than the default value of global `document` - use full sentences with capital first letter and period in the example comments * CR: rename files to kebab-case * CR: await promise -> return promise @kentcdodds: > Rather than `await promise`, I'd prefer `return promise`. Maybe I'm being irrational here, but it feels better to me. @sompylasar: > I'm changing this, but if this line was the only one with `await` expression, then `eslint` would say `async` function must have an `await`. We are lucky that there are more `await`s in all the tests. > > P.S. I don't agree with this rule because `async` functions have their use for the error handling; `async` function is just the one that is wrapped in a `return new Promise(...)`. * CR: shorter timeouts and wait times for quicker tests
so this has been open for almost 5 years, any status updates? |
@mendrik mocking it worked for me https://github.com/benitogf/corsarial/blob/master/test/specs/utils.js#L29 and as mentioned above there's also the polyfill solution, have a good day :) |
We have a partial implementation over here: https://github.com/skatejs/skatejs/blob/master/packages/ssr/register/MutationObserver.js. |
@benitogf I tried that but somehow the records didn't fire for me :( it just didn't throw any errors. |
@mendrik in your tests, import this https://github.com/aurelia/pal-nodejs/blob/master/src/polyfills/mutation-observer.ts |
@mendrik if you're willing to opt-out of JSDOM https://github.com/skatejs/skatejs/tree/master/packages/ssr#usage. If not, then you can import that file directly and add the exports (https://github.com/skatejs/skatejs/blob/master/packages/ssr/register/MutationObserver.js#L109) to your global. |
This was enough to fix this issue for me trying to test a component that extended react-quill:
|
To be clear, jsdom now properly supports MutationObserver after version 13.2.0 released in September 2019. Please remove any workarounds! |
In older versions of JSOM jsdom/jsdom#639 mutation observer is not fully supported and will throw on tests. Wraps the usage of MutationObserver around the `process.env.NODE_ENV` guard to protect for this scenario
In older versions of JSOM jsdom/jsdom#639 mutation observer is not fully supported and will throw on tests. Wraps the usage of MutationObserver around the `process.env.NODE_ENV` guard to protect for this scenario
* feat(waitForElements): add implementation, tests, docs Add `mutationobserver-shim` to `devDependencies` to provide `MutationObserver` in jest tests where `jsdom` has no built-in support for it: jsdom/jsdom#639 * docs(contributors): update sompylasar * CR changes - rename `waitForElements` to `waitForElement` - move `mutationobserver-shim` to dependencies, import from `waitForElement` to provide the polyfill to the users of `dom-testing-library` - fix `kcd-scripts` version to match `master` branch - add synchronous `callback` call to detect an element if it's already present before any DOM mutation happens - add/change tests about synchronous `callback` call - tweak variable names in docs examples - add docs about the default `container` option value - add docs example about querying multiple elements - add docs about the `mutationobserver-shim` polyfill - add docs link and anchor to `mutationObserverOptions` - add docs link to MDN from the second mention of `MutationObserver` * fix(waitForElement): ensure it works with default callback Should wait for the next DOM change, as advertised in the docs. The default value is `undefined` so that the `options` object can be used while still keeping the default callback: ``` waitForElement(undefined, {attributes: true}) ``` * CR: tweak docs examples for wait and waitForElement - use `container` in the examples as this is a more popular use case than the default value of global `document` - use full sentences with capital first letter and period in the example comments * CR: rename files to kebab-case * CR: await promise -> return promise @kentcdodds: > Rather than `await promise`, I'd prefer `return promise`. Maybe I'm being irrational here, but it feels better to me. @sompylasar: > I'm changing this, but if this line was the only one with `await` expression, then `eslint` would say `async` function must have an `await`. We are lucky that there are more `await`s in all the tests. > > P.S. I don't agree with this rule because `async` functions have their use for the error handling; `async` function is just the one that is wrapped in a `return new Promise(...)`. * CR: shorter timeouts and wait times for quicker tests
jsdom 13.2.0 added direct support for MutationObserver, and recommends removing all workarounds. We're on version 19.0. Refs: jsdom/jsdom#639 (comment) Signed-off-by: Mike Fiedler <miketheman@gmail.com>
* refactor: replace deprecated crypto test lib Remove a deprecated library and replace with a native test shim. node-webcrypto-ossl has been deprecated. This module was created in 2015 because at the time the Node team did not feel the need to have two crypto interfaces and they already had one before WebCrypto was defined Refs: https://www.npmjs.com/package/node-webcrypto-ossl Signed-off-by: Mike Fiedler <miketheman@gmail.com> * chore: remove unnecessary mutationobserver-shim jsdom 13.2.0 added direct support for MutationObserver, and recommends removing all workarounds. We're on version 19.0. Refs: jsdom/jsdom#639 (comment) Signed-off-by: Mike Fiedler <miketheman@gmail.com> * chore: remove whatwg-fetch polyfill With `fetch()` being available to all modern browsers, we no longer need to provide this polyfill. Remove related `imports-loader` and `exports-loader` as well. Signed-off-by: Mike Fiedler <miketheman@gmail.com> Signed-off-by: Mike Fiedler <miketheman@gmail.com> Co-authored-by: Ee Durbin <ewdurbin@gmail.com>
https://developer.mozilla.org/en-US/docs/Web/API/MutationObserver
The text was updated successfully, but these errors were encountered: