Version 1.0.1 broke ReScript bindings

[jaidetree]

Working on ReScript bindings for Jotai, I didn't come up with these bindings but have been trying to help maintain them. https://github.com/eccentric-j/re-jotai/tree/stable. They're not complete, or perfect, but the core functionality is there and they pair quite well in so far. As of v1.0.1 though, the get function within a writable atom accepts an optional unstable_promise boolean which broke the bindings as ReScript was returning a function instead of the value of the dependent readable atom. I have a work around by defining a get wrapper that takes two arguments but it does require me to change all my existing writable atoms to call it. It's not ideal but sufficient to unblock progress.

ReScript bindings are likely far from anyone's priority with this project but it's worth recommending a process change to constrain future API changes to major version updates if possible to avoid issues like this. Please note overall I really like Jotai and very much appreciate the work required to make and maintain this library, I don't wish to curb innovation. Since Jotai hit 1.0.0 I am hoping we can find a compromise to avoid breaking dependent libs and bindings from minor version changes.

[dai-shi]

Hi, thanks for your great work! I learned from the ReScript bindings that although jotai api is typescript oriented, its nature is dynamic JS, and not very strict type friendly. Well, I already struggle with atom() function overload with typescript, so I kind of knew.

future API changes to major version updates if possible

I'm afraid this is not possible. The purpose of unstable_promise is to experiment the feature for the next version. I had even considered not exporting the type, but we would also like to experiment TS types, so why not.
Furthermore, even if this becomes stable promise boolean (or it can be { promise: boolean } object),
it won't be major update. It's not breaking change in JS. So, it will be just a minor.
We'd try best to note some clarifications in release notes.

which broke the bindings as ReScript was returning a function instead of the value of the dependent readable atom.

this sounds unfortunate. if there anything we could do without affecting JS and TS, please let us know.

If you are familiar with TS, suggestions to atom() types might help too, not only the write getter. Thanks!

[jaidetree]

Hi, thanks for your great work!

I appreciate that but full credit should go to @gaku-sei for getting the first draft of working bindings out. Not sure I would have known where to even begin, fortunately I just have to keep the bindings in sync with the current version.

I'm afraid this is not possible. The purpose of unstable_promise is to experiment the feature for the next version. I had even considered not exporting the type, but we would also like to experiment TS types, so why not.

Fair enough, restricting to major versions is impractical in this case.

won't be major update. It's not breaking change in JS

That's true it wasn't a breaking change, but it still is an API change. If a major version is unreasonable, could experimental changes be put into a @next tag? I understand your point that the update still fits the original signature but the function.length property still changed behind the scenes.

this sounds unfortunate. if there anything we could do without affecting JS and TS, please let us know.

This error took me about a day to track down. It was a perfect storm where that get function deviating from the get function in a derived atom, and ReScript happens to curry against function.length.

If you are familiar with TS, suggestions to atom() types might help too, not only the write getter. Thanks!

I am thinking of drafting a Jotai-like library in ReScript to see how it would influence the API design. Especially since behavior like:

const writableAtom = atom(
  [],
  (get, set, action) => {
    const state = get(writableAtom);
    switch (action.type) {
      case 'append':
         set(writableAtom, state.concat(action.payload));
         break;
       case 'replace':
         set(writableAtom, action.payload);
         break;
       default:
         // Nothing changes
         break;
    }
  }
)

Is not possible in ReScript.

Perhaps the TS that ReScript generates will be an interesting reference?

[dai-shi]

but the function.lengh property still changed behind the scenes.

interesting.

$ node
Welcome to Node.js v12.20.1.
Type ".help" for more information.
> function x(a) { return a }
undefined
> function y(a,b) { return a+(b||0) }
undefined
> function z(a,b=0) { return a+b }
undefined
> x.length
1
> y.length
2
> z.length
1

So, is it correct if we added the default value, it wouldn't have changed the function.length?

[jaidetree]

I think that depends on how it's transpiled? In modern node, it is natively supported and unreported in the length.

const writableAtom = atom(
  [],
  (get, set, action) => {
    console.log("get length", get.length);
    const state = get(writableAtom);
    switch (action.type) {
      case 'append':
         set(writableAtom, state.concat(action.payload));
         break;
       case 'replace':
         set(writableAtom, action.payload);
         break;
       default:
         // Nothing changes
         break;
    }
  }
)

When using Jotai version 1.0.0 exactly, get length 1 is logged.
When using Jotai version 1.0.1 exactly, get length 2 is logged.

So perhaps either TS or a build setting in package.json is cheesing the optionality of unstable_promise?

EDIT: If perhaps we can normalize the way optionality is supported then it should work just fine with the previous ReScript bindings!

[jaidetree]

Looking a bit deeper into the transpiled JS it looks like the calling code is:

  var writeGetter = function writeGetter(a, unstable_promise) {
    var aState = readAtomState(state, a);

    if (aState.e) {
      throw aState.e;
    }

    if (aState.p) {
      if (typeof process === 'object' && process.env.NODE_ENV !== 'production') {
        if (unstable_promise) {
          console.info('promise option in getter is an experimental feature.', a);
        } else {
          console.warn('Reading pending atom state in write operation. We throw a promise for now.', a);
        }
      }

      if (unstable_promise) {
        return aState.p.then(function () {
          return writeGetter(a, unstable_promise);
        });
      }

      throw aState.p;
    }

    if ('v' in aState) {
      return aState.v;
    }

    if (typeof process === 'object' && process.env.NODE_ENV !== 'production') {
      console.warn('[Bug] no value found while reading atom in write operation. This is probably a bug.', a);
    }

    throw new Error('no value found');
  };

[dai-shi]

would you be able to use esm instead of cjs? cjs is rather for traditional browsers.

[jaidetree]

Looked at esm/index.js and its definition was also:

  const writeGetter = (a, unstable_promise) => {
    const aState = readAtomState(state, a);
    if (aState.e) {
      throw aState.e;
    }
    if (aState.p) {
      if (typeof process === "object" && process.env.NODE_ENV !== "production") {
        if (unstable_promise) {
          console.info("promise option in getter is an experimental feature.", a);
        } else {
          console.warn("Reading pending atom state in write operation. We throw a promise for now.", a);
        }
      }
      if (unstable_promise) {
        return aState.p.then(() => writeGetter(a, unstable_promise));
      }
      throw aState.p;
    }
    if ("v" in aState) {
      return aState.v;
    }
    if (typeof process === "object" && process.env.NODE_ENV !== "production") {
      console.warn("[Bug] no value found while reading atom in write operation. This is probably a bug.", a);
    }
    throw new Error("no value found");
  };

[dai-shi]

Yeah. Here's the one #574.
https://ci.codesandbox.io/status/pmndrs/jotai/pr/574
Please try the codesandbox build. See "Local install instructions".

[jaidetree]

Spent the last 2 hours trying to get that to work. It works in my vanilla js example, but can't get jotai/esm to work with the rescript bindings package.

Tried using native es module support but fails when working with jotai/esm/index.js imports.
Tried using babel which almost worked but seems to be over-transforming the function signature to the 2 arg version. Logging get.length and get.toString() showed the signature as taking two arguments which I know is not what esm/index.js provides.

EDIT: Will keep at it and see if I can find the right combination of config + packages

[jaidetree]

Turned out I had bindings in other files still pointing to regular jotai, replacing those with jotai/esm with babel-env node current preset has it working.

Tomorrow will try your fix in our actual project and see how it all comes together

[fc1943s]

I'm doing some bindings for F#, still far from stable, but encountered a few issues related to the dyn nature of JS too.
It was happening some time ago and I already did some hacks to mitigate that (like useSetStatePrev), but there were occasions where the atom setter functions received a setter function instead of the actual value set by the user, or vice versa, I don't remember.
I don't know if its related, but since its compiled to JS, there is no TS to infer if the atom is declared with a setter fn or actual value? After everything is stable I'll remove the hacks and check if it was just mistakes in my code. I also had to rewrite the useCallback function, will also check if it will work with the original later.
just posting to say that I'm loving jotai so far 😛

https://github.com/fc1943s/fluke/blob/master/src/Fluke.UI.Frontend/src/Bindings/Jotai.fs
https://github.com/fc1943s/fluke/blob/master/src/Fluke.UI.Frontend/src/Bindings/Store.fs

[jaidetree]

Confirmed it is working in our actual app without any tooling changes required. Reports get.length as 1 which means it's not auto-curried. Thanks for taking the time to listen and create that PR!

[dai-shi]

That's good to hear. I will merge the PR.
Please note that this is a subtle thing for us to notice, and we can't guarantee that we could avoid such things in the future, and it's not our priority.