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

Error when adding the 'beforeRecord' event #12

Closed
piotrpalek opened this issue Jun 13, 2018 · 7 comments
Closed

Error when adding the 'beforeRecord' event #12

piotrpalek opened this issue Jun 13, 2018 · 7 comments
Labels
bug 🐞 Something isn't working

Comments

@piotrpalek
Copy link

piotrpalek commented Jun 13, 2018

Hey, I've been testing pollyjs in my Ember app, and it worked fine. Later I've added the beforeRecord event like this:

    server
      .any()
      .on('beforeRecord', (req, recording) => {
        console.log(recording);
      });

and got this error (it goes away when I remove it):

pollyjs-core.js:2 Uncaught (in promise) TypeError: Cannot assign to read only property 'params' of object '#<l>'
    at pollyjs-core.js:2
    at tryCatch (polyfill.js:6900)
    at Generator.invoke [as _invoke] (polyfill.js:7138)
    at Generator.prototype.(:7357/3066/tests/anonymous function) [as next] (http://localhost:7357/assets/vendor.js:12297:21)
    at t (pollyjs-core.js:2)
    at pollyjs-core.js:2
    at new Promise (<anonymous>)
    at new n (pollyjs-core.js:2)
    at pollyjs-core.js:2
    at pollyjs-core.js:2

(btw it'd be nice if source maps would work when in dev mode, so that it would be easier to debug issues like this)

@jasonmit
Copy link
Contributor

jasonmit commented Jun 13, 2018

beforeRecord

If you plan to modify the response body before your XHR request resolves, you'll want to use beforeResponse. In the really near future (today at some point), beforeResponse will be renamed to response.

An example of how to use this event to mutate your response body:
https://github.com/jasonmit/ember-api-docs/blob/u/jasonmit/qunit/tests/acceptance/3-class-polly-advanced-test.js#L13-L25

beforeRecord (soon to be beforePersist) is an event that fires after your XHR requests resolve but before the recording is persisted to disk or local storage. Mutating the response body at this point is too late in the lifecycle which is why we freeze the object.

That said, it may be worth exploring throwing a more meaningful assertion vs exposing the default thrown error from Object.freeze. We've been working the last few days to iron out the event names so that they're hopefully more clear.

(btw it'd be nice if source maps would work when in dev mode, so that it would be easier to debug issues like this)

I can investigate this separately.

@jasonmit
Copy link
Contributor

Feel free to reopen or create a new issue if you have further questions or this doesn't solve what you were trying to do.

@piotrpalek
Copy link
Author

piotrpalek commented Jun 13, 2018

@jasonmit thanks for the answer, I actually wanted to use it to anonymize / remove some data from the recording before commiting it to disk. Would that be the wrong place to do it?

edit: Also why does it throw an error when I only use console.log? Did I use it in the wrong place?

@jasonmit
Copy link
Contributor

@piotrpalek yes I think you want beforeResponse/response for this use-case.

This way your mutations to the response body (not limited to just the body, can include headers for an example) are captured in your test as well as persisted.

@jasonmit
Copy link
Contributor

jasonmit commented Jun 13, 2018

edit: Also why does it throw an error when I only use console.log? Did I use it in the wrong place?

A bug. Reopening. Accessing a frozen object should not have a side effect like this.

@jasonmit
Copy link
Contributor

jasonmit commented Jun 13, 2018

@piotrpalek that error you were seeing about the read-only object should now be fixed on master and will be part of the 0.1.0 release that is going out in a few minutes.

That said, I still believe for your use case you want to use the response event (formerly known as beforeResponse).

@piotrpalek
Copy link
Author

@jasonmit thanks! works great now 👍 also I agree, the response event makes more sense for this use case :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants