Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ObjectAnnotation is incorrect after returning an error in a custom decoder #1224

Open
craig-feldman opened this issue Feb 14, 2025 · 7 comments · May be fixed by #1231 or #1232
Open

ObjectAnnotation is incorrect after returning an error in a custom decoder #1224

craig-feldman opened this issue Feb 14, 2025 · 7 comments · May be fixed by #1231 or #1232
Assignees
@nvie
Labels
bug

Comments

@craig-feldman
Copy link

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
Copy link
Author

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
Copy link
Owner

nvie commented Feb 14, 2025 •
edited
Loading

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 added a commit that referenced this issue Feb 18, 2025
@nvie
Add test cases to replicate bug report #1224
e816147
nvie added a commit that referenced this issue Feb 18, 2025
@nvie
Add test cases to replicate bug report #1224
7487b25
nvie added a commit that referenced this issue Feb 18, 2025
@nvie
Add test cases to replicate bug report #1224
94b1d0f
@nvie
Copy link
Owner

nvie commented Feb 18, 2025 •
edited
Loading

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 nvie self-assigned this Feb 18, 2025
@nvie
Copy link
Owner

nvie commented Feb 18, 2025

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
Copy link
Author

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.

This was referenced Feb 20, 2025
Open
Open
@nvie
Copy link
Owner

nvie commented Feb 20, 2025 •
edited
Loading

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.
  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)?

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

Thanks again for highlighting this issue and reporting it 🙏

@craig-feldman
Copy link
Author

craig-feldman commented Feb 20, 2025 •
edited
Loading

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nvie nvie

Labels
bug
Projects
None yet
Milestone
No milestone
2 participants
@nvie @craig-feldman