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

AST rewriting not adding closing script tag when string is too long #8043

Closed
panzarino opened this issue Jul 20, 2020 · 4 comments · Fixed by #8048 · May be fixed by Omrisnyk/npm-lockfiles#145 or Omrisnyk/npm-lockfiles#164
Closed
Assignees
Labels
experiment: source rewriting Issues when using experimentalSourceRewriting type: bug

Comments

@panzarino
Copy link
Contributor

panzarino commented Jul 20, 2020

Current behavior:

The following test case, with experimentalSourceRewriting turned on, will not properly render the page. (It doesn't properly render the page without it either, see #1068)

  it('can open office online', () => {
    cy.visit('https://www.office.com/launch/excel?ms.officeurl=webapps&auth=2');

    cy.contains('Sign in').should('be', 'visible');
  });

It looks like when the source is being rewritten, something is causing the closing </script> tag to be omitted. When this tag is omitted, the rest of the page goes missing as well.

Here's what the rewritten response looks like:

Screen Shot 2020-07-20 at 6 32 38 PM

Here's the actual response:

Screen Shot 2020-07-20 at 6 32 58 PM

From some initial investigations it appears that this is a bug caused by the length of the script.

@panzarino panzarino added type: bug experiment: source rewriting Issues when using experimentalSourceRewriting labels Jul 20, 2020
@panzarino panzarino changed the title AST rewriting not adding closing script tag AST rewriting not adding closing script tag if script is too long Jul 20, 2020
@panzarino panzarino changed the title AST rewriting not adding closing script tag if script is too long AST rewriting not adding closing script tag Jul 20, 2020
@panzarino panzarino self-assigned this Jul 21, 2020
@jennifer-shehane
Copy link
Member

I've seen an issue with the AUT not loading with this option in a few cases. Hopefully this addresses some of those. 🤞

@panzarino panzarino changed the title AST rewriting not adding closing script tag AST rewriting not adding closing script tag when string is too long Jul 21, 2020
@panzarino
Copy link
Contributor Author

@jennifer-shehane Looks like we're not actually checking that the stream has finished being rewritten before returning the result, meaning long strings will complete after it returns: https://github.com/cypress-io/cypress/blob/develop/packages/rewriter/lib/html.ts#L17. Hopefully fixing with a PR later today.

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Jul 23, 2020

The code for this is done in cypress-io/cypress#8048, but has yet to be released.
We'll update this issue and reference the changelog when it's released.

@cypress-bot cypress-bot bot removed the stage: needs review The PR code is done & tested, needs review label Jul 23, 2020
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Aug 3, 2020

Released in 4.12.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v4.12.0, please open a new issue.

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Aug 3, 2020
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
experiment: source rewriting Issues when using experimentalSourceRewriting type: bug
Projects
None yet
2 participants