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.

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

Add XMP raw string type to ExpandedTags["xmp"] type #197

Merged
merged 1 commit into from
Jul 22, 2022

Conversation

suke
Copy link
Contributor

@suke suke commented Jul 5, 2022

Description

Fix tags.xmp._raw type to string.
Related to #183

Current behavior
Screen Shot 2022-07-05 at 21 38 18

Expected behavior
Screen Shot 2022-07-05 at 21 37 47

@mattiasw
Copy link
Owner

mattiasw commented Jul 5, 2022

Thanks! Will merge at the same time as the other one.

@mattiasw
Copy link
Owner

mattiasw commented Jul 5, 2022

Or actually, this can't make anyone's code start failing during type checking, right? It just expands what is correct.

@suke
Copy link
Contributor Author

suke commented Jul 6, 2022

It probably won't fail the type checking.

However, I found a problem with the combination of intersection types and index signature.
This could be a problem when assigning properties to tags, but I don't think it's a problem if tags are read-only.
microsoft/TypeScript#17867

So far the assignment to tags.xmp seems to be working correctly.

ExifReader.load(file, { expanded: true }).then((tags) => {
  if(tags.xmp) {
    tags.xmp._raw= ''
    tags.xmp['Foo'] = {
      value: '',
      attributes: {},
      description: ''
    }
    tags.xmp['Bar'] = '' // Type 'string' is not assignable to type 'XmpTag'
  }
});

following code has the same problem.
https://github.com/mattiasw/ExifReader/blob/main/exif-reader.d.ts#L184

ExifReader.load(file, { expanded: false }).then((tags) => {
  tags['Foo'] = {
    description: '',
    value: 1
  }
});
// throw an error
// Type 'number' is not assignable to type 'string & XmpTag[] & XmpTags & String'.
// Type 'number' is not assignable to type 'string'.

@mattiasw
Copy link
Owner

Not sure I understand what the problem is. Both of the examples should fail type checking as I see it, and that should be the case even before your change in this PR.

@suke
Copy link
Contributor Author

suke commented Jul 14, 2022

I've confirmed that the first example can't assign a string other than tags.xmp._raw.
This is correct behavior.

As you pointed out, the second exmaple is failing type checking before this PR.
If you do not intend to add properties to tags, there is no problem.

@mattiasw mattiasw merged commit 092b472 into mattiasw:main Jul 22, 2022
@0-CAT
Copy link

0-CAT commented May 31, 2023

This change effectively makes it impossible to create tests/mocks based on XMP tags. After upgrading from 4.5.0, the following code will not compile due to missing _raw:

mockExifLoad.mockResolvedValueOnce({
  xmp: {
    description: { attributes: {}, description: "My description", value: null as never },
  },
});

However, adding _raw: "", to the xmp declaration also causes an error (which is the error discussed above):

image

I understand the idea that tags are supposed to be read-only, but it seems backwards to prevent people entirely from creating them because of the use of this weird edge-case of TypeScript unions.

Is this truly the intended behavior?

@mattiasw
Copy link
Owner

Hi! No, that was not intended. I gave it some time to try and fix it but I can't make it work. Maybe someone with a bit more TypeScript expertise knows how.

@0-CAT
Copy link

0-CAT commented Jun 1, 2023

Yeah, I took a look as well. The link suke posted above was also very helpful. There are a few issues open on the TS project and even some proposals to properly handle this, but it hasn't yet come to a resolution.

It looks like the workaround is actually to do this the way it is, and then use an Object.assign hack to work around it. Something like this:

mockExifLoad.mockResolvedValueOnce({
  xmp: Object.assign({
    _raw: "",
  }, {
    description: { attributes: {}, description: "My description", value: null as never },
  } as XmpTags),
});

I guess we just have to hope for a future TypeScript version that gives us a better way to handle this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants