Skip to content

feat: export FieldProto#8052

Closed
maribethb wants to merge 1 commit intoRaspberryPiFoundation:developfrom
maribethb:field-proto
Closed

feat: export FieldProto#8052
maribethb wants to merge 1 commit intoRaspberryPiFoundation:developfrom
maribethb:field-proto

Conversation

@maribethb
Copy link
Copy Markdown
Contributor

The basics

The details

Resolves

Fixes unexported type

Proposed Changes

Exports the FieldProto type and removes the @internal annotation.

Reason for Changes

FieldProto is used in places where you need to reference a type that refers to a field or any subclass of Field. Since constructors in field subclasses do not all have the same signature, typeof Field is not sufficient as it does not satisfy the TS compiler.

When we added this type, we decided not to export it unless we found that external developers needed it. Code.org is using TypeScript and they have a couple places where they need to refer to "a field or any subclass" so they're currently using this by deep importing the type from blockly/core/field.js but that has never been supported and will throw an error if building with webpack in v11.

Test Coverage

Documentation

This will automatically be added to the reference docs since I removed the @internal annotation.

Additional Information

@maribethb maribethb requested a review from a team as a code owner April 24, 2024 17:59
@maribethb maribethb requested a review from cpcallen April 24, 2024 17:59
@github-actions github-actions Bot added the PR: feature Adds a feature label Apr 24, 2024
Copy link
Copy Markdown
Collaborator

@cpcallen cpcallen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I think exporting this type is probably a bad idea—but I'm also aware that I don't have the full picture here, so let's discuss via VC if we have the chance, so I might better understand what Code.org are up to.

In summary my thoughts (in no particular order) are:

  • FieldProto is a weird and possibly confusing name.
  • It's hard to understand what a FieldProto value is and what one might use it for, and the description in the JSDoc is probably more misleading than helpful.
  • The protected functions that currently take a FieldProto would probably have been better written to take a typeof Field.prototype instead (i.e., the actual prototype object itself, rather than the constructor). That's a breaking change, but now would be a convenient time to make it.
  • Surely TypeScript has some provision for "class types" that does not tie them to the specific calling convention of the constructor???
  • External developers who need a shorthand alias for Pick<typeof Field, 'prototype'> should probably just declare one themselves.

@cpcallen
Copy link
Copy Markdown
Collaborator

Further notes after discussion:

  • I had overlooked that the registry is also using this type. The registry should use a type that captures the things that are actually needed (including e.g. .fromJson), which might also be done via a Pick<> if only certain properties are needed—but if the constructor needs to be called then I'm not sure Pick is the correct approach, as I don't know if the resulting type is a callable constructor.

@maribethb maribethb mentioned this pull request Apr 30, 2024
1 task
@maribethb
Copy link
Copy Markdown
Contributor Author

Closing because we need a different solution, filed #8059 to track

@maribethb maribethb closed this Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: feature Adds a feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants