Extension rewrite, rust cleanup, and new facebook parser #64
Conversation
Gross fix for slow chrome startup.
…al-ads into rust-cleanup
…al-ads into rust-cleanup
|
I'm digging through this now |
|
So what's this Snap thing in the extension tests? |
| onClick={() => dispatch(toggle(type))} | ||
| > | ||
| {getMessage(message)} | ||
| {amount ? <b>{100 > amount ? amount : "100+"}</b> : ""} |
There was a problem hiding this comment.
Is it right that this'll never happen, since the maximum number of ads saved got lowered to 50 (for both Ratings and Ads) as an attempt at a performance fix? Should we maybe move the limit back to 100?
There was a problem hiding this comment.
Yeah I think so
There was a problem hiding this comment.
We should probably make it 150 or something
| } | ||
| } | ||
|
|
||
| menuFilter() { |
There was a problem hiding this comment.
this is intended to be kinda like a Java-style interface, with child classes implementing this, but without a generic implementation?
There was a problem hiding this comment.
StateMachine? Yep that’s just an interface so that the two machines, Parser and IdFinder, conform too
| .setData(parsed) | ||
| .setMethod("GET") | ||
| .setRelativeTo(document.body) | ||
| .setNectarModuleDataSafe(document.body) |
There was a problem hiding this comment.
err, what are setNectarModuleDataSafe and setRelativeTo in particular?
There was a problem hiding this comment.
Those are from Facebook, we call them to get some sort of token that allows us to get the targeting without popping up the huge targeting dialog
|
The snap files are part of jest: https://facebook.github.io/jest/docs/en/snapshot-testing.html They basically make sure there aren’t crazy markup changes |
|
Yeah that’s definitely the craziest part of whole thing because while we’re
on the page we can’t actually interact with JavaScript there so we build
and inject a script tag to store stuff in localstorage. It is the unholiest
hack ever.
…On Thu, Apr 5, 2018 at 11:02 AM Jeremy B. Merrill ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In extension/src/parser.js
<#64 (comment)>
:
> - resolve(ad);
- }
- }
- };
-
- // This is all built out from a close reading of facebook's code.
- let built = grabVariable(
- url => {
- let parsed = new (window.require("URI"))(url);
- localStorage.setItem("url", url);
- let req = new window.AsyncRequest()
- .setURI(url)
- .setData(parsed)
- .setMethod("GET")
- .setRelativeTo(document.body)
- .setNectarModuleDataSafe(document.body)
Oh weird.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#64 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADYRdRNnYiS57WntA0MxD4-x5sJagl3ks5tllwtgaJpZM4THoLI>
.
|
|
I guess let's merge it. I can't find that spot where it cuts the number of ads and ratings down to 50. |
|
Hanging out at the end of mergeAds:
https://github.com/propublica/facebook-political-ads/blob/6f46642fa35be9a0afc169f5a47b22295b0a298b/extension/src/utils.js#L50
…On Thu, Apr 5, 2018 at 11:40 AM Jeremy B. Merrill ***@***.***> wrote:
I guess let's merge it.
I can't find that spot where it cuts the number of ads and ratings down to
50.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#64 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADYRRmags9KIn6BLYHlyMxV0fDG7xusks5tlmUegaJpZM4THoLI>
.
|
|
Ah, cool. It's 200 in this branch (which is fine too!)... which is why I couldn't find it. I'll merge. |
Extension rewrite, rust cleanup, and new facebook parser
This is a very large pull request, because it contains everything from a bit of cleanup on the rust side to a large rewrite of the extension itself. It also includes jest tests for the frontend and more testing for the backend. It should solve a bunch of problems that were documented in #41