mdedetrich / scalajson

ScalaJSON - JSON for Scala, currently contains minimal AST
BSD 3-Clause "New" or "Revised" License
55 stars 10 forks source link

Release 1.0.0 #13

Closed dwijnand closed 6 years ago

dwijnand commented 7 years ago

We need this library to commit to its source and binary API and release a stable 1.0.0 version.

This requires #12 ("New package name and new group id") to have happened.

mdedetrich commented 7 years ago

@dwijnand This is already happening, there is sbt-mima set up to automatically check for binary incompatibilities and the library has been binary compatible for some time.

There is a caveat, and that there may be some design decisions which may need to be revisted

eed3si9n commented 7 years ago

sbt 1.0.0-RC1 is scheduled to be released on July 10th, 2017, which will be bincompat with the final.

Is it possible to finalize Scala JSON (both package name and organization name) in next two weeks?

mdedetrich commented 7 years ago

Package name yes, group id is unlikely unless @jvican manages to release the sbt-platform before then.

dwijnand commented 7 years ago

Do you really need sbt-platform to be released to pick and use a new groupId?

Also, this request is also for this library to commit to its source API, binary API, and API semantics; i.e release its first stable version.

It's a blocking requirement for sbt 1.0.0's release, which is impending.

mdedetrich commented 7 years ago

@dwijnand

Well there is nothing stopping me from using a different group id, but I can't release the package because I don't have deploy permissions for that group id (unless I am missing something), afaik you need some sbt-platform-release plugin being developed by @jvican.

I also believe that the library is still technically in the incubation period.

I have sent @jvican an email

mdedetrich commented 7 years ago

This is now release as 1.0.0-M1, will expect to release 1.0.0 in the next few days if there aren't any issues. Will leave this ticket open if anyone finds anything (will close it on the final release)

gmethvin commented 7 years ago

A couple concerns:

First, I noticed that Map, an abstract trait, is used for JObject while Vector, a final class, is used for JArray. Are we intending to allow custom implementations here or not? Personally I would prefer to use IndexedSeq or Seq for JArray to allow for the same flexibility there.

I also think perhaps we should only include the standard API in this release rather than both standard and unsafe APIs. It seems many are not convinced of the value of this unsafe API, and think we should be conservative about including new things in the platform.

gmethvin commented 7 years ago

I think the immediate problem of having a release that library implementors can work against is solved. I would strongly urge against releasing 1.0.0 until a few libraries have had a chance to integrate this and find any other problems with the library.

gmethvin commented 7 years ago

I also have some concerns about things like the fact that JNumber is based on a string and doing conversion/validation each time it's constructed from a number. I think this would have a non-trivial performance impact and it's something we should consider before making it official API.

Like I've said elsewhere we'll have a much easier time getting this library adopted if existing libraries could implement an interface while still maintaining source and binary compatibility.

mdedetrich commented 7 years ago

First, I noticed that Map, an abstract trait, is used for JObject while Vector, a final class, is used for JArray. Are we intending to allow custom implementations here or not? Personally I would prefer to use IndexedSeq or Seq for JArray to allow for the same flexibility there.

See https://github.com/mdedetrich/scalajson/pull/19. Its likely going to be a custom Map structure which preserves key ordering

I also think perhaps we should only include the standard API in this release rather than both standard and unsafe APIs. It seems many are not convinced of the value of this unsafe API, and think we should be conservative about including new things in the platform.

Commented here https://github.com/playframework/play-json/issues/76#issuecomment-312408829

I think the immediate problem of having a release that library implementors can work against is solved. I would strongly urge against releasing 1.0.0 until a few libraries have had a chance to integrate this and find any other problems with the library.

Honestly I completely agree with you here, and I am bit frustrated here myself. There was a huge period of time where people could give significant feedback, and it was only on the announcement of a prerelease of ScalaJSON that people are suddenly voicing their concerns (most of which valid to a certain degree)

AFAIK, the only thing that is causing the release to be rushed is SBT, @eed3si9n @dwijnand can you comment on how hard the deadline for SBT release is? I would particularly like to get https://github.com/mdedetrich/scalajson/pull/19 done before this release period, and its looking tight

I also have some concerns about things like the fact that JNumber is based on a string and doing conversion/validation each time it's constructed from a number. I think this would have a non-trivial performance impact and it's something we should consider before making it official API.

This is being fixed in this PR https://github.com/mdedetrich/scalajson/pull/24

eed3si9n commented 7 years ago

Honestly I completely agree with you here, and I am bit frustrated here myself. There was a huge period of time where people could give significant feedback, and it was only on the announcement of a prerelease of ScalaJSON that people are suddenly voicing their concerns (most of which valid to a certain degree)

I agree that it feels like a never ending cycle. There's been many opportunities for discussions on all the points that we are being rehashing now from number to insertion order. Literally the second issue opened on this repo was picking one AST instead of two. #2.

If VectorMap drives toward having a single AST instead of two, I think that's a great thing for ScalaJSON. For what it's worth, sjson-new and sbt 1 beta have been using ScalaJSON's unsafe AST, and we haven't run into issues with it.

AFAIK, the only thing that is causing the release to be rushed is SBT, @eed3si9n @dwijnand can you comment on how hard the deadline for SBT release is?

The dates are not solid, but we'll try to stick to our schedule since we want to get sbt 1.0 out soon, and get it in the hands of ppl. For sbt users, my choice of JSON AST is an implementation detail, so I don't think it can be a show stopper.

gmethvin commented 7 years ago

For sbt users, my choice of JSON AST is an implementation detail, so I don't think it can be a show stopper.

@eed3si9n Then I would say just avoid exposing it in the APIs and go with whichever JSON library makes sense for you.

mdedetrich commented 7 years ago

1.0.0-M3 was just released, it contains numerous small fixes. The next milestone will have the safe constructor changes for JNumber, i.e. #28 and #22

eed3si9n commented 7 years ago

Then I would say just avoid exposing it in the APIs and go with whichever JSON library makes sense for you.

@gmethvin sbt 0.13 doesn't expose json4s, but we started having issues when its versions broke bincompat because classpath is shared with plugins. One way to work around this is that everyone forks or shades some version of some AST, but I am hopeful of having a communal JSON AST that keeps bincompat. I think this is achievable, like Vector and String that we can take for granted.

@mdedetrich Do you have a roadmap and schedule towards 1.0?

gmethvin commented 7 years ago

@eed3si9n Ah, that makes sense. If anyone has SBT plugins using JSON it'll probably cause conflicts. Are you planning on only manipulating the raw AST and not using any JSON library on top?

eed3si9n commented 7 years ago

Are you planning on only manipulating the raw AST and not using any JSON library on top?

I yak shaved and wrote an AST-agnostic JSON codec library sjson-new. As long as there's facade for it, I can swap out the backend.

mdedetrich commented 7 years ago

@eed3si9n I am actually currently working on this during my vacation (doing my best to try and get the various issues fixed before the release of SBT 1.x). Currently there appears to be 3 scenarios

  1. Release without VectorMap. This can happen in the next few days. The only outstanding thing is the safe JNumber converters
  2. Release with VectorMap. This is probably going to take a few weeks, as I need to make sure the VectorMap collection is performant and competitive to current Map collection
  3. Release with VectorMap + only single AST. This is possibly an option, it would be a couple ofd ays after the VectorMap

I am going to release 1.0.0-M4 soon, which has the fixes for the JNumber

mdedetrich commented 7 years ago

1.0.0-M4 was released, see this PR for the major changes https://github.com/mdedetrich/scalajson/pull/30

eed3si9n commented 7 years ago

@mdedetrich Thanks for the update. Here is my thinking. I think option 3. is the best thing for ScalaJSON and its future users if we ignore sbt, so I think that's what we should do.

For now, sbt should shade one of your milestones, such as M4, and use that for sbt 1.0.0-RC1 that is due next week. The date may slip based on other development, but shading unblocks us on the JSON front.

After RC-1, there will likely be RC-2 for bug fixes. The constraint is that sbt RCs are bincompat with final, but since AST is not exposed via public API, if ScalaJSON 1.0.0 comes out in time for RC-2, I am willing to do the switch to get back on the proper ScalaJSON. Otherwise, we will attempt the switch in sbt 1.1. What do you think?

mdedetrich commented 7 years ago

After RC-1, there will likely be RC-2 for bug fixes. The constraint is that sbt RCs are bincompat with final, but since AST is not exposed via public API, if ScalaJSON 1.0.0 comes out in time for RC-2, I am willing to do the switch to get back on the proper ScalaJSON. Otherwise, we will attempt the switch in sbt 1.1. What do you think?

This sounds great, I would rather spend the time getting the AST straight rather than rushing a release. Since ScalaJSON isn't going to be exposed that gives us some leeway.

BTW as I discovered you should be able to use the standard AST right now. Even though the immutable.Map doesn't guarantee key ordering in general, it does seem to guarantee key ordering on construction of the initial map. This means that if you don't add/remove elements to the Map, it will maintain its key ordering (this is one of the confusing things I discovered)

gmethvin commented 7 years ago

Even though the immutable.Map doesn't guarantee key ordering in general, it does seem to guarantee key ordering on construction of the initial map.

This is definitely not true:

scala> val letters = Map(('a' to 'z').zipWithIndex: _*)
letters: scala.collection.immutable.Map[Char,Int] = Map(e -> 4, s -> 18, x -> 23, n -> 13, j -> 9, y -> 24, t -> 19, u -> 20, f -> 5, a -> 0, m -> 12, i -> 8, v -> 21, q -> 16, b -> 1, g -> 6, l -> 11, p -> 15, c -> 2, h -> 7, r -> 17, w -> 22, k -> 10, o -> 14, z -> 25, d -> 3)

This is only true for small maps because there are explicit Map1-Map4 types that do retain the ordering:

scala> val letters = Map(('a' to 'd').zipWithIndex: _*)
letters: scala.collection.immutable.Map[Char,Int] = Map(a -> 0, b -> 1, c -> 2, d -> 3)

Either way, I would much prefer having an API that only exposes Map and lets you use any immutable map. The more opinionated this library is about specific data types, the harder it is to adapt existing libraries efficiently.

mdedetrich commented 7 years ago

@gmethvin Yeah it seems to be due to the specializations, although I think I noticed in some cases even with a Map of size greater then 4 it maintained key order, may be due to how you construct the Map (scala collections are complicated, and they can be constructed in a big number of ways)

Either way, I would much prefer having an API that only exposes Map and lets you use any immutable map. The more opinionated this library is about specific data types, the harder it is to adapt existing libraries efficiently.

This is looking likely, although I will see how effective VectorMap is after some specializations. If VectorMap is only a bit slower, having it as the default is a net win because it will prevent this confusing behaviour

eed3si9n commented 7 years ago

We shipped sbt 1.0.0-RC2 today (RC-1 got borked, so it's our first RC) with a shaded ScalaJSON 1.0.0-M4.

Next RC is scheduled to be on July 28th.

SethTisue commented 7 years ago

I'm really, really glad to see that a way is being found to preserve key ordering. 😌

farmdawgnation commented 7 years ago

Wanted to let y'all know that I've opened lift/framework#1897 to add first class support for converting between lift-json and scalajson to the core lift-json library. Once 1.0 is finalize and released here, we'll merge. Presuming that happens soon, I expect this to be included in the Lift 3.2.0 release at the end of the year.

dwijnand commented 6 years ago

This is no longer a requirement for us as sbt, as we ended up forking this library and using a shaded version of it instead for sbt 1:

mdedetrich commented 6 years ago

@dwijnand Is there a plan in the future to use a non shaded/forked version (once scalajson is released properly?)

dwijnand commented 6 years ago

Maybe for sbt 2. We can make that decision closer to the time.