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

Option to skip iframe validation? #60

Closed
markerikson opened this issue Oct 1, 2020 · 6 comments
Closed

Option to skip iframe validation? #60

markerikson opened this issue Oct 1, 2020 · 6 comments

Comments

@markerikson
Copy link

I'm using Penpal in an app, and I'm trying to test that app via Cypress ( https://www.cypress.io/ ).

Unfortunately, Cypress is finicky about dealing with iframes. Part of that is that it appears to modify the value of window.top in order to host your app in its test environment, or something along those lines.

Penpal is currently doing an iframe validation check:

if (window === window.top) {

which is being called unconditionally:

validateWindowIsIframe();

It seems like Cypress's experimentalSourceRewriting option successfully modifies Penpal enough to let the test continue, but that rewriting process takes a very long time to run as the test starts.

I can confirm that if I hand-edit the contents of node_modules/penpal to comment out the validateIframe() check, that my app loads okay in Cypress.

Would it be possible to get an option added to connectToParent() to allow skipping that check? It looks like all that's needed is adding a boolean to the Options type and then moving the validate check into an if statement.

@Aaronius
Copy link
Owner

Aaronius commented Oct 2, 2020

Hey @markerikson, thanks for logging the issue. Will you try (or have you tried) setting modifyObstructiveCode to false in your cypress.json and see if that fixes your issue? We're using Penpal + Cypress on a project at work and I think this is what fixed it in our case.

@markerikson
Copy link
Author

Hmm. Here's what I was trying in the process of getting stuff to work earlier today:

  • Original project setup:
    • penpal@5.1 (or so - still using the version before you went back to publishing ES5-compat syntax)
    • Small cypress.json file, no compilation related options
    • Result: "Must be used in an iframe" error
  • Researched, found Cypress's docs on modifyObstructiveCode
    • Added "modifyObstructiveCode": true to cypress.json
    • result: same "Must be used in an iframe" error
  • Further research, found experimentalSourceRewriting
    • Added "experimentalSourceRewriting": "true" to cypress.json, in addition to "modifyObstructiveCode": true`
    • result: iframe loaded correctly, but Cypress pegged my CPU repeatedly and had very long delays in loading both the original page and the iframe (very understandable if it's parsing and rewriting megabytes of minified JS)
  • Hand-edited the code in node_modules/penpal/ to comment out that validateWindowIsIframe() check and turned off both of those flags
    • result: iframe loaded, no long delays, Cypress test now passes in 15s or so (and most of that is actually exercising the portion of the app in the iframe)

So, I never technically tried "modifyObstructiveCode": false, but that's because it wasn't on in the first place and I assume that false is the default.

Any further thoughts?

@Aaronius
Copy link
Owner

Aaronius commented Oct 2, 2020 via email

@markerikson
Copy link
Author

Huh. Clearly I misread that docs page :) It does indeed say it defaults to true.

Yeah, I just tried uncommenting those lines in node_modules, set "modifyObstructiveCode": false in my cypress.json, and rebuilt the app, and the tests (still) work.

Might still be useful to have this as an option anyway, I guess, but doesn't appear to be necessary to solve this use case now.

Thanks for helping clear that up! Feel free to close this if you don't plan to make the change.

@Aaronius
Copy link
Owner

Aaronius commented Oct 6, 2020

I'm going to update the readme with some of these details, then probably create a new ticket to just remove the check entirely in the next major version since it probably doesn't provide a lot of value anyway.

@Aaronius
Copy link
Owner

I've removed the check from Penpal. This change was released in v6. Thanks!

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

No branches or pull requests

2 participants