8000 fix: export Field-related types from Blockly by btw17 · Pull Request #6877 · google/blockly · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: export Field-related types from Blockly #6877

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
Merged
merged 2 commits into from
Mar 3, 2023
Merged

fix: export Field-related types from Blockly #6877

merged 2 commits into from
Mar 3, 2023

Conversation

btw17
Copy link
Member
@btw17 btw17 commented Mar 3, 2023 •
edited by maribethb
Loading

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide
  • I ran npm run format and npm run lint

The details

Resolves

Re-export Field types from Blockly
fixes #6870

Proposed Changes

  • Adds FieldDropdownConfig export
  • Imports then re-exports Field{Name}Config and Field{Name}FromJsonConfig types from ./core/blockly.ts

Behavior Before Change

  • Config exports were not re-exported in ./core/blockly.ts

Behavior After Change

  • Config exports are re-exported in ./core/blockly.ts

Reason for Changes

  • To make the included types available in the root export for Blockly

Test Coverage

N/A

Documentation

N/A

Additional Information

  • FieldDropdownConfig is just a re-export of FieldConfig. Should we include this?
    • We currently have FieldTextInputConfig as a re-export of FieldInputConfig already, though the base FieldInputConfig isn't actually exported since FieldInput is only used internally
  • Note: As mentioned above, the base FieldInput types aren't exported. It is not a registered field since it was created for internal use when splitting the old FieldInput to FieldTextInput. I don't think these types should be exported, though it's worth noting that this is deliberate.
  • In ./core/blockly.ts, think it's worthwhile to export UnattachedFieldError or FieldProto?
    • FieldProto isn't necessarily needed, though it could theoretically be used by and edge-case subclass that makes significant changes upon the base Field class

@btw17 btw17 requested a review from a team as a code owner March 3, 2023 19:20
@btw17 btw17 requested a review from BeksOmega March 3, 2023 19:20
@github-actions github-actions bot added the PR: chore General chores (dependencies, typos, etc) label Mar 3, 2023
@maribethb
Copy link
Contributor

Changes so far lgtm, regarding the questions (which can be addressed in this PR if you like)

  • FieldInput and its related objects should all be marked @internal if they are intentionally not exported. This removes them from the documentation.
  • I think FieldProto should also be marked @internal and not exported, based on how we're currently using it. If needed someone can file a fr and ask us to change this in the future
  • Yes, let's export UnattachedFieldError

@btw17
Copy link
Member Author
btw17 commented Mar 3, 2023

Should be all set @maribethb! I did as you suggested with a few other notes:

  • I moved Validator types to the bottom of their files so match the convention with the Config files
  • I removed a redundant comment line for the validator in core/field.ts
  • I copied the validator function comment to each redeclared type
  • I removed the export for InputTypes in ./core/field_inputs.ts since it's not exported in core/blockly.ts and not used in any other files

@maribethb maribethb self-requested a review March 3, 2023 21:37
@maribethb maribethb assigned maribethb and unassigned BeksOmega Mar 3, 2023
@maribethb maribethb removed the request for review from BeksOmega March 3, 2023 21:37
@github-actions github-actions bot added PR: chore General chores (dependencies, typos, etc) and removed PR: chore General chores (dependencies, typos, etc) labels Mar 3, 2023
@maribethb maribethb changed the title chore: re-exported Field types from Blockly fix: export Field-related types from Blockly Mar 3, 2023
@github-actions github-actions bot added PR: fix Fixes a bug and removed PR: chore General chores (dependencies, typos, etc) labels Mar 3, 2023
@maribethb
Copy link
Contributor

Note: Changed the PR title, because I think this should be reflected in release notes (chores are excluded)

Also updated PR comment to close the linked issue.

@maribethb maribethb merged commit 9e5bfc2 into google:develop Mar 3, 2023
@btw17 btw17 deleted the chore/export-blockly-field-types branch March 10, 2023 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maribethb maribethb maribethb approved these changes

Assignees

@maribethb maribethb

@BeksOmega BeksOmega

Labels
PR: fix Fixes a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Re-export Field types from Blockly
3 participants
@btw17 @maribethb @BeksOmega
0