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

Update uBlock Origin to use npm package #2138

Merged
merged 1 commit into from
Aug 10, 2021

Conversation

mjethani
Copy link
Contributor

@mjethani mjethani commented Aug 9, 2021

Using the new StaticNetFilteringEngine class from @gorhill/ubo-core.

@mjethani
Copy link
Contributor Author

mjethani commented Aug 9, 2021

@gorhill please take a look first. It seems to be blocking more requests now! Try to clone https://github.com/mjethani/adblocker and switch to the update-ubo-core branch.

@gorhill
Copy link
Contributor

gorhill commented Aug 9, 2021

Weird, I will investigate. What was the previous version?

On my side I used built-in benchmark to compare current master with 1.37.3b4 and I got same result.

@mjethani
Copy link
Contributor Author

mjethani commented Aug 9, 2021

What was the previous version?

@@ -1 +1 @@
-Subproject commit 543e1a3aea0c775ee34b7f168388a9ebaef1b4c5
+Subproject commit d9adf5a6fb688d470e67204501fdee176f9b4d4a

If it helps, I tried with enableWASM() and I also checked that the type value is being propagated to the engine. This must be something wrong with the way I'm using it here or otherwise with the changes that involve the moving around of the parser, compiler, etc.

@mjethani
Copy link
Contributor Author

mjethani commented Aug 9, 2021

This works until gorhill/uBlock@65f0909

@mjethani
Copy link
Contributor Author

mjethani commented Aug 9, 2021

This works until gorhill/uBlock@65f0909

By this I mean that it works in that commit too. It probably broke in the one that followed. Let me confirm this.

@mjethani
Copy link
Contributor Author

mjethani commented Aug 9, 2021

This works until gorhill/uBlock@65f0909

By this I mean that it works in that commit too. It probably broke in the one that followed. Let me confirm this.

Confirmed. It broke in gorhill/uBlock@7cd583a. In that commit it blocks 85,131 requests. It should block 82,388 requests.

@gorhill
Copy link
Contributor

gorhill commented Aug 9, 2021

When I compare on my side using built-in benchmark which uses all default filter lists, I get 94,560 for both before and after that commit. This suggests the issue is in a code path not used by the extension.

@mjethani
Copy link
Contributor Author

mjethani commented Aug 9, 2021

When I compare on my side using built-in benchmark which uses all default filter lists, I get 94,560 for both before and after that commit. This suggests the issue is in a code path not used by the extension.

It's almost certainly in platform/nodejs/index.js.

@gorhill
Copy link
Contributor

gorhill commented Aug 9, 2021

Looks like loadJSON('build/publicsuffixlist.json') is returning an empty string.


Right, it's empty, it's just "".

@mjethani
Copy link
Contributor Author

mjethani commented Aug 9, 2021

Looks like loadJSON('build/publicsuffixlist.json') is returning an empty string.

gorhill/uBlock#3807

@mjethani mjethani marked this pull request as ready for review August 9, 2021 23:20
@mjethani mjethani requested a review from remusao as a code owner August 9, 2021 23:20
@mjethani
Copy link
Contributor Author

mjethani commented Aug 9, 2021

@gorhill @remusao fixed now and ready for review.

@mjethani mjethani changed the title Update uBlock Origin to d9adf5a Update uBlock Origin to 6b993f2 Aug 9, 2021
@mjethani
Copy link
Contributor Author

mjethani commented Aug 9, 2021

@gorhill if you publish a new version to npm, I'll update this patch to use the npm version directly just as we do with adblock-rs and @adguard/tsurlfilter.

@gorhill
Copy link
Contributor

gorhill commented Aug 9, 2021

@gorhill if you publish a new version to npm, I'll update this patch to use the npm version

Done: https://www.npmjs.com/package/@gorhill/ubo-core

@mjethani mjethani changed the title Update uBlock Origin to 6b993f2 Update uBlock Origin to use npm package Aug 9, 2021
@mjethani
Copy link
Contributor Author

mjethani commented Aug 9, 2021

@gorhill if you publish a new version to npm, I'll update this patch to use the npm version

Done: https://www.npmjs.com/package/@gorhill/ubo-core

@remusao I've updated the patch now to remove the git submodule and use the npm package directly.

@remusao remusao merged commit bda0c4b into ghostery:master Aug 10, 2021
@mjethani mjethani deleted the update-ubo-core branch August 10, 2021 06:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Internal 🏠 Changes only affect internals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants