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

Fix: inject function options dictionary has wrong type definition #6231

Merged
merged 1 commit into from
Jun 23, 2022

Conversation

andyanday33
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

Behavior Before Change

The inject function was giving an error while trying to add a JSON toolbox inside an options object. Yet it was working fine after compiling to javascript.

error1

Behavior After Change

Typescript stopped showing any errors after the change. The function runs perfectly fine.

inject

Reason for Changes

The inject function requires a JSON toolbox to include blocks inside the workspace. Since the function is giving an error when including a toolbox in options, it may be misleading for some.

@andyanday33 andyanday33 requested a review from a team as a code owner June 21, 2022 14:49
@google-cla
Copy link

google-cla bot commented Jun 21, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Copy link
Contributor

@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.

Thanks for this. The signature we published is definitely wrong (it looks like our tooling got confused about how the BlocklyOptions interface type is declared in the Closure type system) and your update is much more correct.

I'm going to go ahead and merge this (once test pass), though I should note that we're in the middle of a big migration from JS to TS, which (if successful) means that our next release will actually have typings generated from TypeScript sources. Still: if that doesn't come to pass this PR will at least make sure that what we do release is a little less wrong than before!

@cpcallen cpcallen merged commit 233cce8 into google:develop Jun 23, 2022
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.

None yet

2 participants