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

Fix: Update browser extension to use a copy of the DOM #1922

Closed
wants to merge 7 commits into from

Conversation

sarvaje
Copy link
Contributor

@sarvaje sarvaje commented Feb 15, 2019

Pull request checklist

Make sure you:

For non-trivial changes, please make sure you also:

  • Added/Updated related documentation.
  • Added/Updated related tests.

Short description of the change(s)

This PR contains the same commits as #1910 but also add the code to share the code for traverse. The only code to review/comment is 9d40fa6

With these changes, the extension bundle size goes from 6.76MB to 11.4MB. If we stub some packages as request and tough-cookie we can save 1.1MB in the bundle, but we will probably need to work a little bit in the stubs to avoid errors.

@molant
Copy link
Member

molant commented Feb 16, 2019

the extension bundle size goes from 6.76MB to 11.4MB

😱

@molant
Copy link
Member

molant commented Feb 16, 2019

Have you done any tests to measure the difference with this approach in a heave website?

@antross
Copy link
Member

antross commented Feb 16, 2019

@molant - Discussing with @sarvaje earlier I was debating whether we should hold off on migrating the extension until we can move parser-html and traversal support to something lighter than jsdom (e.g. once cheerio has been fixed to have location information - or using the underlying dependencies that cheerio uses directly).

We certainly aren't getting much code savings in this case - seems hard to justify the extra weight at the moment.

And yes, we absolutely should test the before/after to see if there's a noticable performance impact on heavy websites (if we decide to move forward).

@molant
Copy link
Member

molant commented Feb 16, 2019

I think the biggest reason to use this approach was for performance so we should test if this actually improves it or not. If it does then we should start looking into optimizing the bundling process. IIRC we weren't minifying the code and we can probably be more agressive with other configurations.

@antross
Copy link
Member

antross commented Feb 16, 2019

Currently it won't be a performance win as the existing code was able to traverse the native DOM directly in one shot. That changes if/when we decide to move analysis in extension-browser to a worker, though.

@molant
Copy link
Member

molant commented Feb 16, 2019

Maybe we should evaluate contributing back to cheerio to get the location fixed sooner rather than later?

@antross
Copy link
Member

antross commented Feb 16, 2019

Personally I think we may be better off using the underlying components. We don't need much more than basic DOM data and a selectors API, both of which are provided by dependencies of cheerio (htmlparser2, domhandler, domutils, and css-select to be specific - all developed to work together). The jQuery emulation is unnecessary overhead for us.

@molant
Copy link
Member

molant commented Feb 16, 2019

'head :first-child' is the most complicated selector we are using (meta-charset-utf-8) so I guess underlying should be fine.

@sarvaje
Copy link
Contributor Author

sarvaje commented Feb 19, 2019

Maybe we should evaluate contributing back to cheerio to get the location fixed sooner rather than later?

cheerio already has this fixed, but is going to be part of the v1.

@molant
Copy link
Member

molant commented Feb 25, 2019

Superseeded by #1943

@molant molant closed this Feb 25, 2019
@sarvaje sarvaje deleted the extension-shadow-dom branch May 30, 2019 17:26
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

Successfully merging this pull request may close these issues.

3 participants