-
Notifications
You must be signed in to change notification settings - Fork 422
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
Use Piped's fork of nanojson #981
base: dev
Are you sure you want to change the base?
Conversation
Also, our fork of nanojson was outdated, while FireMasterK's is not
You also need to update
|
Yeah sorry, I was about to do that |
I also include fastutil (which is not a problem for Piped since I use all over in its code), but is an additional dependency, maybe we don't want to include that commit here? |
Dang bro that's pretty slow. I'm not really sure why I was requested though. But anyway, if it runs fine and better, then it looks good to me 👍 It'll probably also need to be changed here too, right? |
I don't understand why the mocks are missing. That's an indicator for changes in the requests, most likely those which have a JSON body. We need to update the mocks and should have a brief look on how the requests changed exactly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above
I had a look into this, and now keys for the objects generated just aren't sorted:
New:
YouTube doesn't order fields, so I feel this may indeed be a good thing I think we should regenerate the mocks No clue what they changed for this to happen tho: TeamNewPipe/nanojson@0185616...FireMasterK:nanojson:master~2 |
Wouldn't this make mocks useless?
The only change which could make a difference in my opinion is FireMasterK/nanojson@6b8c1ef. Also, why not updating our fork instead of using a fork of our fork? |
No, because now they're in the order that we add them in the builder I believe
That could be done as well, but how would you remove the old commits? I'm not sure what's the team's policy on force pushing... Edit: Or actually, we could create a new branch and rename this one |
Then, that's fine for me! |
That would be fine, but since FireMasterK's fork is actively maintained and serves almost the same purpose, I think it would make more sense to just use that. It wouldn't be difficult to switch away if the needs ever diverged. We could archive our fork in the meantime. |
Hi, the context is here: #232 (comment). I believe that Bandcamp has switched to valid JSON in the meantime, without any comments. :) |
I would also be in favor of updating our fork instead. I also think using lazy parsing of e.g. numbers would be great.
There were 2 reasons for our fork of nanojson:
|
I'd like to upstream as many changes as possible from my fork! It becomes lesser work for me to maintain my fork then anyway. A lot of my changes could probably be useful for TeamNewPipe's fork, but not upstream nanojson due to some limitations by my changes. I've made a lot of performance-oriented changes in my fork in comparison to upstream. I've also added support for what I call "Lazy String" parsing in my fork. This could probably even be upstreamed to the original repository even. (I don't have the bandwidth for that currently) I'd like to express my dislike for the nanojson library for doing all the parsing eagerly. If feasible, I would even like to replace it in the extractor with a more performant and well-built library like Jackson. |
Our fork of nanojson was initially introduced in #317. That PR contains no description, but if I remember correctly our own fork was created in order to be able to parse some particular Json format returned by some service.
Anyway, I have run the tests with Piped's / @FireMasterK's fork and there does not seem to be any problem. The upstream library (i.e.
com.grack.nanojson
) cannot be used directly since it returnsnull
instead ofnew JsonObject()
in some methods, causing NPEs (fixed by7056f30
in our fork). In any case, @FireMasterK's PR is up-to-date with upstream (last upstream commit is Aug 9), contains7056f30
, and contains one more commit related to performance (https://github.com/FireMasterK/nanojson/commits/master). Since our fork changed some things related to how the JSON is parsed, it turned out to be thousands of times slower than upstream: see this analysis by @FireMasterK.These are the only tests that fail: they are unrelated to the changes in this PR, since they are the same as here.