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

The great Blockly upgrade #9865

Merged
merged 110 commits into from
Feb 21, 2024
Merged

The great Blockly upgrade #9865

merged 110 commits into from
Feb 21, 2024

Conversation

riknoll
Copy link
Member

@riknoll riknoll commented Feb 13, 2024

It's time!

This PR upgrades us to the latest version of Blockly and removes our dependency on our fork (pxt-blockly). I went over this in the tech talk, but the basic gist of what this PR does:

  1. Converts pxtblocks/ into modules
  2. Moves all of the functionality that was in pxt-blockly into plugins. These are in the pxtblocks/plugins/ folder
  3. Removes all of the many, many places where we were using private methods or monkey patching Blockly. There is still one monkey patch in BlockSvg, but I'm looking to get rid of it.

Here are some test links (open in incognito):

https://arcade.makecode.com/app/c5c20c036100e88b7eeca290470f8c45c41118f9-9f2b2b0e96

https://makecode.microbit.org/app/f9ad2136c00106a54df0b4686be1651471195d35-00171f0e8a

There are a few known minor bugs that were discovered in testing today. @Jaqster and @srietkerk will be filing them in pxt-microbit so that they're tracked.

Known regressions (will file bugs):

  1. Workspace search (ctrl+f) doesn't have the nice rendering of the live site. I need to investigate this plugin to see if I can fix it (or if I need to fork it)
  2. No reindeer connectors. This will be re-enabled when Blockly 10.4 ships later this week
  3. No duplicate on drag shadow blocks. The Blockly team is working on this and it will be in the next major release
  4. Workspace comments have the default rendering. The Blockly team is working on this and it will be in the next major release.
  5. In context translations for blocks are currently disabled. I'll re-enable these soon

Future work items:

  1. Fix above regressions
  2. Stop using our own custom field registry and move to Blockly's field registry
  3. Move more of our fields into plugins and break the plugins into their own repo
  4. Migrate our .blocks files from XML to JSON serialization

@abchatra
Copy link
Collaborator

Can we update the Major version of PXT and the target after we merge this. Once we do that we should update the browerDbPrefixes for microbit so as to ensure projects are not completely corrupted once they try out beta

    "browserDbPrefixes": {
        "1": "v1",
        "2": "v2"
    },

Instructions here:
https://github.com/microsoft/pxt-backend/blob/master/node/backend/docs/versioning.md#target-branch-for-yearly-releases

Copy link
Collaborator

@eanders-ms eanders-ms left a comment

Choose a reason for hiding this comment

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

LGTM :). Made a small comment on package dependencies.

package.json Outdated
@@ -66,13 +66,14 @@
"npm": ">=8.0.0"
},
"dependencies": {
"@blockly/keyboard-navigation": "^0.1.18",
"@blockly/plugin-workspace-search": "^4.0.10",
"@blockly/keyboard-navigation": "^0.5.4",
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we aren't checking in package-lock.json, we need to lock all dependencies to specific versions.

"@types/fuse": "2.5.2",
"@types/highlight.js": "9.12.2",
"@types/jquery": "3.3.29",
"@types/marked": "0.3.0",
"@types/mocha": "2.2.44",
"@types/mocha": "^2.2.44",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is risky unless we check in package-lock.json. This introduces uncertainty around what we develop against vs. what we ship with.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is now a good time to include package-lock.json?

Copy link
Member Author

Choose a reason for hiding this comment

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

one thing at a time! but i will revert this, not sure how it even happened

@riknoll riknoll merged commit 16f4fb2 into master Feb 21, 2024
6 checks passed
@riknoll riknoll deleted the dev/riknoll/the-great-blockly-swap branch February 21, 2024 22:34
@THEb0nny
Copy link
Contributor

@riknoll tell me, for example with this code...
categoriesDiv.style.backgroundColor = (this.sourceBlock_ as Blockly.BlockSvg).getColourTertiary();

Now, do you need to specify this for Blockly.BlockSvg?
import * as Blockly from "blockly";

@riknoll
Copy link
Member Author

riknoll commented Mar 19, 2024

@THEb0nny you might be better off not taking this change... This was a pretty huge update to Blockly and I expect that all the pxt-ev3 fields will require a major rewrite.

To answer your question, though, you don't want to import blockly here as the dependency graph can get a little weird. There are two functions defined in pxt for requiring blockly and pxtblockly:

const pxtblockly = pxt.blocks.requirePxtBlockly()
const Blockly = pxt.blocks.requireBlockly();

This will import the runtime code, but not the types themselves. Some of the casts being done might throw errors

@THEb0nny
Copy link
Contributor

THEb0nny commented Mar 19, 2024

@riknoll I'm already doing something.
I understand from other projects that fieldeditors are not used again. For example, in microbit there is only one fieldeditors, and in arcade there are none at all. I was thinking, if I remove the field_port field, which by and large completely repeats pxtblockly.FieldImages, and in blocks using ports I set images this.fieldEditor="images". Then I will need to write in all files with blocks in which field_port was used these values this.fieldOptions.columns="4" this.fieldOptions.width="300" so that the view will be the same as before the change? Or is there some other way?

/// <reference path="../node_modules/pxt-core/localtypings/pxtblockly.d.ts"/>

const pxtblockly = pxt.blocks.requirePxtBlockly()
const Blockly = pxt.blocks.requireBlockly();

export interface FieldPortsOptions {
    columns?: string;
    width?: string;
}

export class FieldPorts extends pxtblockly.FieldImages {
    public isFieldCustom_ = true;

    constructor(text: string, options: FieldPortsOptions, validator?: Function) {
        super(text, options as any, validator);

        this.columns_ = parseInt(options.columns) || 4;
        this.width_ = parseInt(options.width) || 300;

       this.updateSize_ = (Blockly.Field as any).prototype.updateSize_;
    }
    
}

@THEb0nny
Copy link
Contributor

THEb0nny commented Mar 19, 2024

To answer your question, though, you don't want to import blockly here as the dependency graph can get a little weird. There are two functions defined in pxt for requiring blockly and pxtblockly:

Why am I asking about import * as Blockly from "blockly"
There are field_colors there in the opt_validator?: Function constructor.

constructor(text: string, params: FieldColorEnumOptions, opt_validator?: Function) {
         super(text, params, opt_validator);

         this.paramsData = params["data"];
     }

Error in super method for opt_validator.
An argument of type "Function" cannot be assigned to a parameter of type "FieldValidator".
The "Function" type does not provide a match for the signature "(newValue: any): any".

I googled somewhere for the solution opt_validator?: Function. Found opt_validator?: Blockly.FieldValidator
But then the error is on Blockly, although I indicated above

const pxtblockly = pxt.blocks.requirePxtBlockly()
const Blockly = pxt.blocks.requireBlockly();

but when I add
import * as Blockly from "blockly";
and if I remove const Blockly, the error disappears.

Tell me how to solve this problem correctly.

image

I found it from here.
https://github.com/microsoft/pxt/blob/master/pxtblocks/fields/field_toggle.ts

@riknoll
Copy link
Member Author

riknoll commented Mar 19, 2024

the pxt.blocks.requireBlockly function only imports the runtime code and not type definitions like Blockly.FieldValidator. as a temporary solution, i would just declare the type as any until i figure out how to best reference those types without importing Blockly itself. if you import Blockly here, you will end up including blockly in the page more than once which could impact perf/cause weird things to happen.

and yes, wherever possible you should just use the fields that are defined in pxt (such as FieldImages) and not pxt-ev3. it might be worth looking into if there is a way to deprecate all of them; this will likely change how the blocks look/function but will certainly make keeping up with latest pxt less of a headache.

@THEb0nny
Copy link
Contributor

THEb0nny commented Mar 19, 2024

@riknoll (this.sourceBlock_ as Blockly.BlockSvg) can it be replaced this.sourceBlock_ as any?

@riknoll
Copy link
Member Author

riknoll commented Mar 19, 2024

@THEb0nny i'm afraid not, there were major changes made to Blockly in the version we updated. many of the methods/properties we were using before either no longer exist or were made private (and thus inaccessible). if you are getting an error for something on Blockly.BlockSvg, I think it's likely something that was removed in that way and you need to either figure out the new way to access those properties or re-implement it from scratch.

this is what i meant when i said that all of the field editors will require a major rewrite (if you want to use version 10.X.X of pxt).

alternatively, you can take a dependency on the v9 branch of pxt which uses the old version of Blockly.

@THEb0nny
Copy link
Contributor

THEb0nny commented Mar 20, 2024

@riknoll well, I made almost all the changes.
microsoft/pxt-ev3#1052

I had to import BlockSvg } this way.

import { BlockSvg } from "blockly";

The only problem left is with field_color. The choice of value does not work there, but there is no error.
For some reason the className attribute is not set.

There is something wrong with the colornumber field with the class setting. Because the class is not installed.

//% blockId=colorEnumPicker block="%color" shim=TD_ID
    //% weight=0 blockHidden=1
    //% color.fieldEditor="colornumber"
    //% color.fieldOptions.colours='["#f12a21", "#ffd01b", "#006db3", "#00934b", "#000000", "#6c2d00", "#ffffff", "#dfe6e9"]'
    //% color.fieldOptions.columns=3
    //% color.fieldOptions.className='legoColorPicker'
    export function __colorEnumPicker(color: ColorSensorColor): number {
        return color;
    }

image

@THEb0nny
Copy link
Contributor

@riknoll In your field_colors showEditor_() was removed, where the class was installed. Was this done intentionally?

showEditor_() {
            super.showEditor_();
            if (this.className_ && this.picker_)
                pxt.BrowserUtils.addClass(this.picker_ as HTMLElement, this.className_);
        }

@THEb0nny
Copy link
Contributor

I fixed all the fields pxt-ev3.

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.

4 participants