Update nodes.ts to allow strong NodeType

[fezproof]

Changes

Issue: NodeType in NodeBase always resolves to be potentially undefined.

Solution: Update the NodeBase type to have a stronger NodeType field that can be undefined only if you don't specify it.

Use case

Current behaviour

Desired behaviour

This also now works for union types:

Version info

I consider this type issue a bug, but you probably want to link it to a minor version update to not shock people if the type suddenly change. Up to you though :)

[changeset-bot]

⚠️ No Changeset found

Latest commit: 9f7bc8c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[moklick]

Hey @fezproof thanks for your input here. Why did you close the PR?

[fezproof]

Hey @fezproof thanks for your input here. Why did you close the PR?

Sorry didn’t mean to!

[bcakmakoglu]

My 2 cents: this change would mean that user could not omit type anymore (which probably isn't a big deal for most users) to use the default node type without explicitly defining it as default.

For example:

    { id: 'a', type: 'input', position: { x: 0, y: 0 }, data: { label: 'wire' } }, // <--- this node is fine
    { id: 'b', position: { x: -100, y: 100 }, data: { label: 'drag me!' }, }, // <--- this node gives an error because `type` property is missing

[moklick]

I checked this again and unfortunately we can't make this change because it would be a breaking change. In a lot of examples (and real world apps), nodes do not have a type and all those apps would break.

[fezproof]

Let me have another crack at this, I am sure I can get this to work

[fezproof]

@moklick @bcakmakoglu I have taken a second pass at this and updated the generic to be able to handle both defined and undefined NodeType. If undefined, it will fall back to the original definition.
All the repo now pass typechecks + it builds and passes tests.

[fezproof]

Hey @moklick and @bcakmakoglu, does those changes fix your concerns?

[peterkogo]

@fezproof Can you please explain what undefined extends NodeType really does? Or do you have some pointer to the official documentation.

[RichSchulz]

@fezproof Can you please explain what undefined extends NodeType really does? Or do you have some pointer to the official documentation.

The way I understand this it checks whether the type provided as NodeType is undefined (i.e. no type provided) or can be undefined (e.g. "input" | "output" | undefined), and then propagates this information correctly to the type property of the NodeBase type (be either making it optional or not).

@fezproof is this what you intended?

[RichSchulz]

Hey @moklick are there more concerns from your side regarding this PR? This PR would make upgrading to xyflow 12 for us much more easy :)

[moklick]

As @RichSchulz pointed out, this makes the migration to v12 easier for some users so we should check this again,

[moklick]

Thanks again @fezproof ! Took a bit longer than expected, but this will land in the next release 🚀
And thanks @RichSchulz for the kindly reminder :)

[moklick]

finally merged and released with 12.7.0 🙌 Thanks @fezproof!