ObjectAnnotation is incorrect after returning an error in a custom decoder

[craig-feldman]

Description of the bug
When using the define decoder and returning an error, the resultant error generated after calling this custom decoder with invalid input seems incorrect. This happens when the custom decoder returns the error generated from a built in decoder.

Expected behavior
Returning an error in this case should result in the same error being thrown when calling this custom decoder.

define((blob, ok, error) => {
  const a = decoderA.decode(blob);
          if (!a.ok) {
              return error(a.error);
          }
...
}

To Reproduce
We have a custom decoder defined as follows for combining objects:

export function combineObjects<
    A extends Record<string, unknown>,
    B extends Record<string, unknown>,
>(decoderA: Decoder<A>, decoderB: Decoder<B>): Decoder<A & B> {
    return define((blob, ok, error) => {
        const a = decoderA.decode(blob);
        if (!a.ok) {
            // console.log(util.inspect(a, { depth: null }));
            return error(a.error);
        }

        const b = decoderB.decode(blob);
        if (!b.ok) {
            return error(b.error);
        }

        return ok({ ...a.value, ...b.value });
    });
}

This can be called as follows:

const profile = {
    name: 'Bob',
    surname: 'Burgers',
    age: 40,
    occupation: 'Restaurateur',
};

const restaurantOwner = combineObjects(
    object({
        name: nonEmptyString,
        surname: nonEmptyString,
        age: nonEmptyString,
    }),
    object({
        age: positiveInteger,
        occupation: oneOf(['Restaurateur']),
    })
);

const result = restaurantOwner.decode(profile);

expect(result.ok).toBe(false);
expect(result.value).toBeUndefined();

// console.log(util.inspect(result, { depth: null }));
// The next expectation passes, but I would think that it should actually be expect(result.error?.fields.get('age')).toStrictEqual({
expect(result.error?.text!.fields.get('age')).toStrictEqual({
    text: 'Must be string',
    type: 'scalar',
    value: 40,
});

The first console log (where we return the error) produces a correctly formatted error:

{
  ok: false,
  value: undefined,
  error: {
    type: 'object',
    fields: Map(4) {
      'name' => { type: 'scalar', value: 'Bob', text: undefined },
      'surname' => { type: 'scalar', value: 'Burgers', text: undefined },
      'age' => { type: 'scalar', value: 40, text: 'Must be string' },
      'occupation' => { type: 'scalar', value: 'Restaurateur', text: undefined }
    },
    text: undefined
  }
}

but at the second console log, the error looks incorrect:

{
  ok: false,
  value: undefined,
  error: {
    type: 'object',
    fields: Map(4) {
      'name' => { type: 'scalar', value: 'Bob', text: undefined },
      'surname' => { type: 'scalar', value: 'Burgers', text: undefined },
      'age' => { type: 'scalar', value: 40, text: undefined },  <----- Looks wrong
      'occupation' => { type: 'scalar', value: 'Restaurateur', text: undefined }
    },
    text: { <----- Looks wrong
      type: 'object',
      fields: Map(4) {
        'name' => { type: 'scalar', value: 'Bob', text: undefined },
        'surname' => { type: 'scalar', value: 'Burgers', text: undefined },
        'age' => { type: 'scalar', value: 40, text: 'Must be string' },
        'occupation' => { type: 'scalar', value: 'Restaurateur', text: undefined }
      },
      text: undefined
    }
  }
}

Additional context
Decoders version: 2.6.0
TypeScript version: 5.7.3
Flow version: NA
Node version: 20.18.2

This was working for us before in version 2.0.3

[craig-feldman]

Also picked up that if one changes from restaurantOwner.decode(profile); to restaurantOwner.verify(profile);, then the decoders package itself throws an error:

TypeError: s.indexOf is not a function
 ❯ isMultiline ../../../node_modules/.pnpm/[email protected]/node_modules/decoders/dist/index.cjs:105:12
 ❯ serializeAnnotation ../../../node_modules/.pnpm/[email protected]/node_modules/decoders/dist/index.cjs:228:42
 ❯ formatInline ../../../node_modules/.pnpm/[email protected]/node_modules/decoders/dist/index.cjs:234:36
 ❯ format ../../../node_modules/.pnpm/[email protected]/node_modules/decoders/dist/index.cjs:301:21
 ❯ Object.verify ../../../node_modules/.pnpm/[email protected]/node_modules/decoders/dist/index.cjs:324:13

[nvie]

Thanks for opening this ticket and writing up the extensive report. I will take a look at this soon. Bit of busy this weekend, so it will likely be next week.

[nvie]

Hi @craig-feldman! I finally had the time to investigate here, but I cannot seem to replicate the issue myself. Your decoder definitions seems right to me, no issues there.

I added the test case expectations and code snippet you shared to the test suite but… they are all passing for me when I try it 🤔
Could you take a look here and tell me what I'm doing wrong to replicate your issue? Thank you — I want to make sure this gets resolved for you! 🙏

[nvie]

One theoretical way there an issue like this could get created might be if somehow you have multiple versions of decoders that are being mixed. Can you confirm you only have one version of decoders installed?

npm ls decoders

Does this report more than 1 version by any chance?

[craig-feldman]

That's for following up on this @nvie . Can confirm that replicating that test case for us also passes. The issue may have to do with our project structure and setup. We use pnpm workspaces and a monorepo. We have defined the combineObjects decoder in a shared workspace package (@shared/decoders). After further investigation we have picked up that when import combineObject via the package name (from another service in the monorepo), we get this behaviour and the results are incorrect:

// inside some other service/package
import {combineObjects} from '@shared/decoders/general`

However, when we change that to an absolute/relative import e.g. import { combineObjects } from '../../../decoders/src/general'; then it works as expected.

Can confirm that we only have references to decoders 2.6.0 and no other versions. It looks like the issue may be unique to our setup, but we cannot figure out why return error(a.error); produces an incorrect result in the former, but correct result in the latter. It's obviously something to do with absolute imports versus importing via a package import, but I wouldn't expect there to be any difference.

[nvie]

If there are only 2.6 versions, it's still possible to end up with multiple copies of decoders in your bundle, if somehow (due to your configuration) you end up in your bundle with both a CJS and an ESM "copy" of decoders. The reason it's causing issues is that decoders uses a module level weakset to track whether a value is an Annotation or not (without having to look at or rely on the presence of specific fields). This weakset is supposed to be global singleton, but if you have two copies of decoders in your bundle (one from the CJS and one from the ESM module) then annotations created by the CJS "copy" will not be recognized by the ESM "copy" as such. In those cases, the annotations are recognized as a user value incorrectly. So then it would be annotating the annotation instead of the input object. I think this is what you're seeing, and it's absolutely a bug.

I've provided two potential ways to fix this. Would you be able to try these out and see which one fixes your issue best?

1. Register the internal weaksets under a global symbol, so the registry truly is global, even if there happens to be both a CJS and an ESM copy in your bundle. Decoders will still be doubly-included in your bundle, but you wouldn't notice it, because it would still just work.
   • PR: Ensure internal registries are truly global singletons #1231
   • To try it out: npm i https://pkg.pr.new/decoders@4bb6c36
2. Drop CJS support and go ESM-only. Will this fix your issue (or show a bundling error so that you may better understand if/where there is a CJS/ESM issue)?
   • PR: Drop CJS support, make it ESM-only #1232
   • To try it out: npm i https://pkg.pr.new/decoders@ee81aac

Curious to hear which of these will best fix your issue!

Thanks again for highlighting this issue and reporting it 🙏

[craig-feldman]

Thank you for the detailed feedback @nvie! I was thinking that it might have something to do with the WeakSet, but wasn't sure how to dive further into that. It does look like the build for us outputs both cjs and esm. I'll try out your suggestions and revert back to you soon. Thanks!