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

feat: make field initView and initModel more accessible #7345

Merged
merged 3 commits into from
Aug 8, 2023

Conversation

maribethb
Copy link
Contributor

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

Fixes inconsistency in our API. The init method on a field is @sealed and @internal. The documentation says to override initView and initModel instead. However, those methods were also marked @internal meaning they couldn't actually be overridden.

Proposed Changes

  • Make initView protected because it's only used inside subclasses of fields
  • Make initModel public because it's also used from within block serialization code
  • Refactor usage of initModel in block serialization code because the old style of for loop was causing the field to be typed as any which breaks reference searching. Now, "find all references" will actually find them all.
  • Makes imageElement and imageHeight protected in field_image. Having the image element be @internal makes the image field almost impossible to subclass. If you want to use dynamic image fields that can change images, you have to subclass the image field.

Behavior Before Change

  • Unable to subclass certain fields effectively

Behavior After Change

  • Able to subclass fields and change their dom structure

Reason for Changes

  • Improving documentation and making our API more consistent
  • Code.org needs to be able to subclass image field for dynamic images

Test Coverage

All tests pass and manually tested serialization in playground.

Documentation

These methods will be included in the next documentation update through api-documenter

Additional Information

@maribethb maribethb requested a review from a team as a code owner August 1, 2023 00:55
@github-actions github-actions bot added the PR: feature Adds a feature label Aug 1, 2023
@BeksOmega
Copy link
Collaborator

For background: This happened because these used to be @package which automatically got converted to @internal, but they have slightly different semantics.

@package means: You can override this externally! But please don't call it externally, we will call it internally (from somewhere outside the class).
@internal means: You don't know this exists externally. You cannot override it or call it. We will call it internally (from somewhere outside the class).

So for places where we expected people to override methods, but not call them, the @internal tags are incorrect.

@maribethb maribethb merged commit a0b5657 into google:develop Aug 8, 2023
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.

3 participants