[BREAKING] Package rewrite for a 1.0 release #374
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #374 +/- ##
==========================================
- Coverage 99.35% 93.11% -6.25%
==========================================
Files 7 7
Lines 466 1176 +710
==========================================
+ Hits 463 1095 +632
- Misses 3 81 +78 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
quinnj
force-pushed
the
jq-1.0
branch
7 times, most recently
from
55b8d07 to
84c670b
Compare
| - `omit_empty::Bool` whether empty Julia collection values should be skipped when serializing | ||
| - `allownan::Bool` similar to the parsing keyword argument to allow/disallow writing of invalid JSON values `NaN`, `-Inf`, and `Inf` | ||
| - `ninf::String` the string to write if `allownan=true` and serializing `-Inf` | ||
| - `inf::String` the string to write if `allownan=true` and serializing `Inf` |
There was a problem hiding this comment.
possibly inf_string? Also do we need ninf or can it just print as a - before inf?
| - Explicit keyword arguments to control a number of serialization features, including: | ||
| - `omit_null::Bool` whether `nothing`/`missing` Julia values should be skipped when serializing | ||
| - `omit_empty::Bool` whether empty Julia collection values should be skipped when serializing | ||
| - `allownan::Bool` similar to the parsing keyword argument to allow/disallow writing of invalid JSON values `NaN`, `-Inf`, and `Inf` |
There was a problem hiding this comment.
also, should this be allownonfinite?
There was a problem hiding this comment.
JSON3.jl had allow_inf, but JSON.jl already had allownan, so I opted to keep that for backwards compat.
There was a problem hiding this comment.
I do get the backwards compat, but as a potential consumer of the API I will say that allownan makes me think that the keywords is only for NaN, not Inf.
There was a problem hiding this comment.
Could it be worth checking how many packages depend on the current JSON.jl to estimate how breaking any "breaking changes" might be in practice?
|
Tiny detail but these methods don’t close the file handle: Line 172 in c9d0b1b |
|
Interesting situation w/ the Documentation job failure here: Documenter depends on JSON, but doesn't have compat w/ 1.0, so it can't install. Would we need to have Documenter.jl pre-emptively support 1.0? Other solutions? |
|
JSON ends up being a dependency in most environments. Is there any significant precompile / load times changes with this PR? |
|
As [noted](JuliaIO/JSON.jl#374 (comment)), the JSON 1.0 release is currently blocked on using Documenter due to the circular dependency of needing Documenter to build docs. The usage of JSON.jl in Documenter.jl is very vanilla, so this PR proposed "preemptive" support for JSON.jl 1.0 since the usage of JSON in Documenter is known to not rely on any breaking changes proposed.
* Preemptively support JSON 1.0 release As [noted](JuliaIO/JSON.jl#374 (comment)), the JSON 1.0 release is currently blocked on using Documenter due to the circular dependency of needing Documenter to build docs. The usage of JSON.jl in Documenter.jl is very vanilla, so this PR proposed "preemptive" support for JSON.jl 1.0 since the usage of JSON in Documenter is known to not rely on any breaking changes proposed.
Just pushed a fix; thanks for mentioning. |
|
Having to uncompress all the test files for all package updates feels like it could be slightly slow (at least on Windows). In TOML (which has a similar test suite) I get them during testing from an archive (https://github.com/JuliaLang/TOML.jl/blob/44aab3c023323587680ab8d8c7ef478bf78c4c0c/test/utils/utils.jl#L8) but I guess an archive could also be checked in. Alternatively, all the content of the files could be put into one file with a sentinel "file separator" and get constructed on the fly during testing. |
quinnj
force-pushed
the
jq-1.0
branch
3 times, most recently
from
48aad24 to
3716320
Compare
Great callout; I refactored so that we have tar files of both |
What about just getting each file from the tarball directly (no on-filesystem untaring) using Tar.jl? |
Ah, can Tar.jl do that? I was going off the README docs and I didnt' see anything about that, but that would indeed by nice. |
Ok, I looked into |
|
You can create a |
quinnj
force-pushed
the
jq-1.0
branch
3 times, most recently
from
95eb152 to
6a892cb
Compare
|
A bit confrontative perhaps, but why not a JSON4.jl? To me, looking at the development history, there is nothing really that suggests a JSON5.jl is not in the pipeline (or will that be an update to JSON2.jl?). It seems reasonable to leave this package be quite basic in its functionality that satisfies a lot of people while keeping the JSONX.jl for increasing X being for those that want the most recent cool stuff in JSON parsing. |
It's a fair comment and one I expected. Here's a few thoughts in response:
For all those reasons, I'm still going to push for this and I think it's the right direction for the ecosystem overall. Let's not let the "default" JSON.jl stay dated and stale, but let's breathe some new life into it and push things into the future with better performance, flexibility, while maintaining the simplicity in the code. I'm actually experiencing some major FOMO this week in not being able to attend JuliaCon 2025, so to somewhat "tag along virtually" I had a goal of pushing this over the finish line and making a big announcement. The only remaining thing I've been digging into is the Arrow.jl + JSON.jl extension compatibility and I think I'm really close. |
|
@maleadt can we add this repo to nanosoldier so we can run a PkgEval on this? |
|
Aight, finally ironed out the Arrow + JSON extension and added it here + some tests. Yeah, I'm down for a pkgeval run if that's not too hard. I also should have a decent amount of time over the next week to help with migration efforts. We have the migration guide as part of the docs and I wanted to also start going around and helping to make PRs to update packages as that can uncover a few other things sometimes. I anticipate this will be similar to other large major releases I've helped with in the past for HTTP.jl, CSV.jl, etc. |
|
I thought with
it meant that this would be non-breaking with only features on top of it. If this is breaking and requires a major update I see almost no value in doing this change into JSON over another JSON package. Like, who does it benefit? This is literally a whole new package, no? Regarding
Surely, the answer to a lack of maintainership isn't to significantly increase the amount of code and functionality? |
Sure. Somebody with admin permissions here should reach out to me on Slack. |
It's 99% non-breaking. And the breaking part (custom serialization context overloads), are much better served in the new framework. Most packages won't have to do anything to update. Packages using JSON3.jl will have a slightly bigger upgrade path swapping JSON3 for JSON and StructTypes w/ StructUtils, but even that is very simple and could easily be automated or AI-assisted.
Is the answer not to actually welcome new maintainership? It's not, IMO, a significant increase in code (<2x), but brings a lot of bug fixes and new, great functionality. I think I can see the argument of "if it's not broke, don't fix it", but I think I'm trying to counter that we should do this upgrade/release because:
|
|
My 20c opinion: As a simple comparison, HDF5.jl makes sense, JLD2.jl makes sense, JSON4.jl or JSON5.jl does not. |
|
I'm in support of merging this PR.
The original problem was that JSON was split into JSON2 and JSON3. Maybe there were reasons, but it's also natural that the best code is not available in the beginning. Yes, there will be some pain to transition back to JSON.jl, but my 2c would be to restore JSON.jl and archive both JSON2 and JSON3. That way the community can work towards one implementation for one idea "JS Object Notation" -- this is better than trying to maintain 3,4,5.. repos for the same concept.
Acknowledged that strict semver requires 100% compliance, but in that case again JSON2 and JSON3 should never have happened. |
|
Given this is a major version change, aren't breaking changes fine, just best minimised without compromising the overall sanity of the new API? |
|
I'm concerned users like VSCodeServer will struggle with this change because they need to vendor all of their dependencies and this new version has more dependencies. It makes sense to me to have JSON.jl be the "best JSON package for most users" but it seems that JSON.jl is currently "the best JSON package for dependency-sensitive, performance-insensitive users" and such users should be provided a transition plan. Maybe pre-v1 JSON.jl can be spun out into a new SimpleJSON.jl or something? |
| @@ -1 +0,0 @@ | |||
| @test Float32(JSON.parse(json(2.1f-8))) == 2.1f-8 | |||
There was a problem hiding this comment.
many of these tests look like they could be kept
There was a problem hiding this comment.
I removed a lot of extraneous tests where they were already covered by existing, more consolidated tests (as opposed to one-offs). It seemed like a good time to "re-baseline" some of these issue-specific tests that are now just "standard covered". We also now run both the jsonchecker and JSONTestSuite testing suites, which are industry best-practice for JSON corner cases.
|
I agree that API breakages are fine and expected. Since this is a pre-1.0 package, people depending on this package should expect an unstable API. My main issue with this PR is that it currently relies on a large number of undocumented features from Julia, which makes reviewing the code almost impossible for someone like me who isn't familiar with how Julia is implemented, or how it will change in the future. I also don't think mmap should be used in this package. Many commonly used systems do not have mmap available, or working well, and at least on my machine, julia> @be _ Mmap.mmap("foo.json") x->GC.gc() seconds=100.0
Benchmark: 710 samples with 1 evaluation
min 206.200 μs (22 allocs: 1.273 KiB)
median 268.750 μs (22 allocs: 1.273 KiB)
mean 298.476 μs (22 allocs: 1.273 KiB)
max 691.200 μs (22 allocs: 1.273 KiB)
julia> @be _ read("foo.json") x->GC.gc() seconds=100.0
Benchmark: 798 samples with 1 evaluation
min 158.300 μs (16 allocs: 800 bytes)
median 203.700 μs (16 allocs: 800 bytes)
mean 205.402 μs (16 allocs: 800 bytes)
max 317.600 μs (16 allocs: 800 bytes)
julia> filesize("foo.json")
5
julia> versioninfo()
Julia Version 1.11.6
Commit 9615af0f26 (2025-07-09 12:58 UTC)
Build Info:
Official https://julialang.org/ release
Platform Info:
OS: Windows (x86_64-w64-mingw32)
CPU: 16 × 12th Gen Intel(R) Core(TM) i5-1240P
WORD_SIZE: 64
LLVM: libLLVM-16.0.6 (ORCJIT, alderlake)
Threads: 1 default, 0 interactive, 1 GC (on 16 virtual cores) |
I've added a new |
I've appreciated your thorough review of this PR so far. I've tried to address all of your concerns of "julia internals" usage by putting in proper guards where appropriate and raising issues in Julia proper to try and get better public APIs for things. I think I can also get behind your thoughts on Mmap. It's been a pattern of mine for years now to reach for it in file-byte-parsing scenarios, but I think overall |
|
Usage of and dependency on Mmap has now been removed. |
…ionality that can be directly vendored in
|
|
||
| # hand-rolled scoped enum | ||
| module JSONTypes | ||
| primitive type T 8 end |
There was a problem hiding this comment.
Using primitive type instead of just struct here seems ugly and unnecessary.
|
I think the For example, function json(a, indent=nothing)
if isnothing(indent)
writefile(String, a)
else
String(push!(writefile(Vector{UInt8}, a; pretty=convert(Int, indent)), UInt8('\n')))
endThe julia> @test JSON.json("hello world", 2) == "\"hello world\"\n"
Error During Test at REPL[12]:1
Test threw exception
Expression: JSON.json("hello world", 2) == "\"hello world\"\n"
MethodError: json(::String, ::Int64) is ambiguous.
Candidates:
json(fname::String, obj; kw...)
@ JSON ~/github/JSON.jl/src/write.jl:428
json(a, indent::Integer)
@ JSON ~/github/JSON.jl/src/JSON.jl:107
Possible fix, define
json(::String, ::Integer)
Stacktrace:
[1] macro expansion
@ ~/.julia/juliaup/julia-1.11.6+0.x64.linux.gnu/share/julia/stdlib/v1.11/Test/src/Test.jl:677 [inlined]
[2] top-level scope
@ REPL[12]:1
ERROR: There was an error during testing
|
This is a proposal to overhaul the package internals, providing mostly the same high-level interfaces (
JSON.parse,JSON.json, etc) while also adding a number of enhancements (lazy parsing support, typed materialization similar to JSON3).My goal is to deprecate JSON3.jl in favor of the simpler, faster, and overall better functionality as implemented in this PR.
I've reviewed all existing open issues/PRs to both this package and JSON3.jl, marking all that will be fixed or resolved with the functionality as proposed (over 70 issues resolved in total!).
I've tried to provide extensive new documentation (in the new docs/ directory) on all proposed functionality, including a migrate.md file that provides a detailed migration guide for both JSON.jl pre-1.0 users and JSON3.jl users.
My hope and aim is that the net upgrade for the vast majority of JSON pre-1.0 users will simply be adding
JSON = "1"in their compat files. I've tried to support as much as possible in what I could tell was part of the JSON.jl public API. A few things were deliberately left out as my impression is they were either part of an ancient API that can be better served by more modern Julia mechanisms, or just seem not very useful. If there are things we discover feel too breaking, I'm definitely open to try and find ways to add backwards compatibility.Another part of my commitment in this proposal, if/when merged and released, is to take time across the ecosystem to help upgrade packages. I've done this for other packages (CSV.jl, DataFrames.jl, HTTP.jl, etc.) with big releases, and I feel it helps with a new release "momentum" to get a bunch of key packages upgraded on the new release.
This proposal does add another dependency on the newish StructUtils.jl package and I'll try to provide a little context/justification. I've actually been working on the core refactor of this code and StructUtils.jl for around 2 years. The ideas and redesign came from architectural pains/complexities in JSON3.jl and StructTypes.jl, and a desire to find an internal framework that was at least as powerful/flexible as what ST + J3 provided, but in a vastly simpler way (the JSON3.jl code kept growing in complexity and became hard to work with/maintain/improve). I believe the design of StructUtils.jl to be much less invasive and natural when working with Julia structs, while the power of the core functional parsing methods provide a clean integration for handling 1) default materialization from JSON, 2) typed materialization, and 3) simple serialization from structs and a variety of other types. What's more, is the StructUtils.jl machinery generalizes easily beyond just JSON, and I already have other unreleased packages that use it for database model interactions, struct diffing, and a number of other use-cases.
As for review, I recognize that huge PRs like this are very hard to break down and see the exact net changes. In this case, the entire package is pretty much rewritten, so I'd recommend checking out the PR locally, testing out code, and reviewing the entire package/files in that way (i.e. trying to look at any kind of "diff" on github isn't probably that useful).
I'd also like to propose a public "community review" time where I would host a google meet call that would be open to anyone interested and I would take time to go over package internals/architecture and help answer questions/concerns in person. If folks are interested, please ping me in the public Julia slack or comment here and I'll arrange a time that works for those interested.