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

Implements uBO style polyfills for requests redirects #29

Merged
merged 9 commits into from Jun 18, 2019

Conversation

@AndriusA
Copy link
Collaborator

AndriusA commented Jun 6, 2019

returns request content to use for requests matched on rules with redirect option

Closes #27

@AndriusA AndriusA requested a review from pes10k Jun 6, 2019
@AndriusA
Copy link
Collaborator Author

AndriusA commented Jun 6, 2019

@snyderp if you review tests/ublock-coverage.rs and the tests in src/resources.rs it should be clear what gets handled - please check it is all as intended.

Also added a test that checks parsing extracts all 110 resources for the snapshot of resources.txt included in the repo

Copy link
Collaborator

pes10k left a comment

I think it looks great! Thats a gadzooks for getting this done. One other request; could you update the README.md or similar to show how to use the feature? Or just what end points to use to consume the new resources file?

}
Some(data_url.trim().to_owned())
} else {
// TOOD: handle error - throw?

This comment has been minimized.

Copy link
@pes10k

pes10k Jun 6, 2019

Collaborator

Could this be logged somehow, even if its just mirroring the "didn't understand filter: X" stuff the current lib does? Would be a nice, noisy reminder if there is some new filter format we don't support, something like that

@pes10k
Copy link
Collaborator

pes10k commented Jun 6, 2019

Also, is this exposed through the node API?

AndriusA added 2 commits Jun 6, 2019
Updates README to demonstrate redirect handling, includes logging to stderr for some of the worse failure cases
@AndriusA AndriusA force-pushed the feature-redirect-injection branch from 6ae14dd to d6530eb Jun 6, 2019
AndriusA added 2 commits Jun 10, 2019
@AndriusA
Copy link
Collaborator Author

AndriusA commented Jun 12, 2019

@snyderp should we merge and close? It is backwards-compatible, so don't see any harm in that, FFI and browser can be wired up in due course

@pes10k
Copy link
Collaborator

pes10k commented Jun 12, 2019

Yep, looks good to me, though it looks like there are some conflicts now. But I think the change is terrific!

Make sure deserialization is backwards-comaptible by not deserializing redirect polyfill resources
@AndriusA
Copy link
Collaborator Author

AndriusA commented Jun 13, 2019

Merged master to resolve conflicts

Also to ensure deserialization is backwards compatible have to ignore deserializing of redirect resources for now - included the required changes, tagging @bbondy for review

@AndriusA AndriusA requested a review from bbondy Jun 13, 2019
@AndriusA
Copy link
Collaborator Author

AndriusA commented Jun 18, 2019

Switching to MessagePack for serialization to have a cleaner way of adding new fields to blocker structures over time - this works as intended with #serde(default) field property by using default value for the field if it isn't found when deserializing.

@AndriusA AndriusA force-pushed the feature-redirect-injection branch from 63c2eb4 to 7d64942 Jun 18, 2019
@bbondy
bbondy approved these changes Jun 18, 2019
@AndriusA AndriusA merged commit 2a1a19b into master Jun 18, 2019
4 checks passed
4 checks passed
Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@bbondy bbondy mentioned this pull request Nov 12, 2019
5 of 32 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.