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

Remove regex-based JS/HTML rewriting, switch to AST-based rewriting #7297

Open
15 tasks
flotwig opened this issue May 11, 2020 · 7 comments
Open
15 tasks

Remove regex-based JS/HTML rewriting, switch to AST-based rewriting #7297

flotwig opened this issue May 11, 2020 · 7 comments
Labels
experiment: source rewriting Issues when using experimentalSourceRewriting pkg/proxy This is due to an issue in the packages/proxy directory pkg/rewriter This is due to an issue in the packages/rewriter directory stage: ready for work The issue is reproducible and in scope type: enhancement Requested enhancement of existing feature

Comments

@flotwig
Copy link
Contributor

flotwig commented May 11, 2020

AST-based JS/HTML rewriting is available as an experimental feature with #5273

Here are the remaining tasks before it can fully replace the existing regex-based rewriting:

  • Validation with users - is it solving the issues we set out to fix?
  • (2) Rewrite all traffic (currently, only AUT's current origin is intercepted)
    • may require inlining resolvers.ts
  • (3) Do URL scheme not considered when deciding if origin has changed or not #7268 or have it be fixed by inlining resolvers.ts
    • or else rewriting will break if a user visits an http:// url that redirects to https://
  • (4) Inject HTML via parse5 instead of regex.
  • (4) Resolve issue where proxy will no longer work for normal web browsing outside of Cypress frame. Possible solution: inject a stub window.Cypress.resolveWindowReference into every html page here:
    const { oneLine } = require('common-tags')
    module.exports = {
    partial (domain) {
    return oneLine`
    <script type='text/javascript'>
    window.Cypress = {
    // so window.top, etc. still work when browsing outside of Cypress
    resolveWindowReference: (w, o, v) => o[v];
    }
    document.domain = '${domain}';
    </script>
    `
    },
    full (domain) {
    return oneLine`
    <script type='text/javascript'>
    document.domain = '${domain}';
    var Cypress = window.Cypress = parent.Cypress;
    if (!Cypress) {
    throw new Error('Something went terribly wrong and we cannot proceed. We expected to find the global Cypress in the parent window but it is missing!. This should never happen and likely is a bug. Please open an issue!');
    };
    Cypress.action('app:window:before:load', window);
  • (4) Performance measurements:
    • difference between rewriting with sourcemap and without
    • comparison to regex-based rewriting
  • (4) Performance improvements:
    • improve speed: add streaming parsing/rewriting of JS (parse5 handles rewriting HTML as a stream)
    • mem usage: explore caching js in deferred-sourcemap-cache on filesystem
    • understand how sourcemaps are cached in browser and how we can reduce unnecessary regeneration/not store old sourcemaps in RAM
  • (4) Add comments around injected code to help users understand - potentially include original code in a comment
  • (1) Need a real fix for Electron worker_threads SIGABRT when calling process.exit with worker_threads active electron/electron#23366
  • (4) Fix all code TODOs from original PR: Rewrite JS/HTML using AST-based approach #5273
@flotwig flotwig added type: enhancement Requested enhancement of existing feature pkg/proxy This is due to an issue in the packages/proxy directory pkg/rewriter This is due to an issue in the packages/rewriter directory labels May 11, 2020
@cypress-bot cypress-bot bot added the stage: ready for work The issue is reproducible and in scope label May 11, 2020
@jennifer-shehane jennifer-shehane added the type: breaking change Requires a new major release version label May 12, 2020
@flotwig flotwig removed the type: breaking change Requires a new major release version label May 15, 2020
@drumbeg
Copy link

drumbeg commented May 29, 2020

I am using expermentalSourceRewriting in 4.7.0 and getting the error

 /root/.cache/Cypress/4.7.0/Cypress/Cypress[40]: ../../third_party/electron_node/src/node_worker.cc:386:virtual node::worker::Worker::~Worker(): Assertion `thread_joined_' failed.
azure-ci_1   | The Test Runner unexpectedly exited via a exit event with signal SIGABRT

Is this a known issue and is there a workaround? This causes our tests to fail in CI even when we have a fully passing suite.

@flotwig
Copy link
Contributor Author

flotwig commented May 29, 2020

@drumbeg Does it happen while exiting the tests? If so, that is a known bug, caused by some issue in Electron: electron/electron#23366 Cypress uses a workaround, but it seems to be failing in your case. Does it fail every time?

If it happens before the end of the test suite, that's a new bug, in which case please open a new issue and tag me there to track it.

Either way, debug logs would be helpful from your test run. You can enable debug logs w/ environment variables:

  • On Linux/macOS: DEBUG=cypress:* cypress ...
  • On Windows: npx cross-env DEBUG=cypress:* cypress ...

Make sure to scrub any private data from the logs before sharing here.

@flotwig flotwig added the experiment: source rewriting Issues when using experimentalSourceRewriting label May 29, 2020
@drumbeg
Copy link

drumbeg commented May 29, 2020

It only happens when exiting the tests and crucially it only happens when running within a custom built Docker container based on Oracle Linux 7 (based on RHEL). I get the failure every time, again, only at the end of the suite.

I'm having other issues when trying to attach the debugger.

Is there anything I should be wary of when running in Docker using an image based on RHEL?

@flotwig
Copy link
Contributor Author

flotwig commented May 29, 2020

Is there anything I should be wary of when running in Docker using an image based on RHEL?

Not to my knowledge.

@drumbeg
Copy link

drumbeg commented Jun 3, 2020

@flotwig shall I add debug logs to issue #7572 or are they not useful at this point? Looks like you are underway with looking at a workaround.

@flotwig
Copy link
Contributor Author

flotwig commented Jun 3, 2020

@drumbeg yeah, just merged that PR; it may fix the issue for you once released

@drumbeg
Copy link

drumbeg commented Jun 9, 2020

4.8.0 looking good 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
experiment: source rewriting Issues when using experimentalSourceRewriting pkg/proxy This is due to an issue in the packages/proxy directory pkg/rewriter This is due to an issue in the packages/rewriter directory stage: ready for work The issue is reproducible and in scope type: enhancement Requested enhancement of existing feature
Projects
None yet
Development

No branches or pull requests

3 participants