microsoft / PowerBI-visuals-tools

Contains tools for building/packaging Power BI visuals
https://www.powerbi.com
MIT License
330 stars 149 forks source link

Unknown type(s) in VisualUpdateOptions #422

Closed dm-p closed 1 year ago

dm-p commented 2 years ago

Hi folks - I've noticed that since sometime late last week in the developer visual, an unknown VisualupdateType of 126 was being supplied in the VisualUpdateOptions, typically when I'd expect an All to be supplied. This has since changed to 254. Neither of these are known to the visual API so they cannot be easily handled and I'm having to cast the values to strings so I can inspect them.

This is playing havoc the code I'm using to ensure I route my logic correctly, which is now starting to look like this:

switch (true) {
    case options.type === VisualUpdateType.All:
    case options.type === VisualUpdateType.Data:
    // TODO: there's something funny going on with the developer visual
    case options.type.toString() === '254':
    case options.type.toString() === '126': {
   ...
}

Is this a bug, or are you intending to update the API at some point to reflect and expose these unknown types?

Related: I've created a PR for a similar issue some time ago (microsoft/powerbi-visuals-api#38) for the ResizeEnd event, which is also incorrectly coded. Is it possible for someone in the team to review and approve? I also need this in my visuals for similar reasons, and I'm currently having to cast.

dm-p commented 2 years ago

For completeness, I'll just add I'm using powerbi-visuals-tools 4.0.5 + powerbi-visuals-api 3.8.4 (I have checked through to 4.2, which is the latest published version and 4.4, which is the version in the repo's main branch).

voroshagyma commented 2 years ago

I can confirm that a new VisualupdateType has appeared with the value of 254 and it caused some issues in our application. We are using powerbi-visuals-tools@3.4.3 and powerbi-visuals-api@3.8.4

anupamacloudst commented 2 years ago

Same issue in powerbi-visuals-api@4.4 . Our visual stop working and have to add this types in checks

saviourofdp commented 1 year ago

I believe you need to use the value as bitwise flags, ie

if (options.type & powerbi.VisualUpdateType.All) { /* ... */ };
if (options.type & powerbi.VisualUpdateType.Data)  { /* ... */ };
if (options.type & powerbi.VisualUpdateType.Resize)  { /* ... */ };
if (options.type & powerbi.VisualUpdateType.ResizeEnd)  { /* ... */ };
if (options.type & powerbi.VisualUpdateType.Style)  { /* ... */ };
if (options.type & powerbi.VisualUpdateType.ViewMode)  { /* ... */ };
dm-p commented 1 year ago

I'm now seeing a new unknown/unsupported type in the developer visual this morning - 510. I'm currently using API 5.1 and there's nothing in the VisualUpdateType enum for this in the latest API (5.2). Or even in the PR for 5.3.

Is this intentional? If so, can you please provide the details of what it means? Introducing these new, unknown update types is causing all kinds of havoc with those of us that are trying to use these update types to drive our visual logic.

These should really only be introduced if we're using a visual API version that supports their types. Otherwise, it would be better to just return VisualUpdateType.All if this is unknown to older API versions.

Please also consider dedicating some time to fixing the current outstanding issues with this enum for developers:

Demonkratiy commented 1 year ago
Hello colleagues, sorry for this confuse. First of all, there was inconsistency in extending of `VisualUpdateType` in PowerBI backend and in API. We will release API 5.3.0 where we will fix this very soon. Next, I want to clarify how `VisualUpdateType` works and why it uses such values. So, it uses binary system "under the hood" to manage type flags. This table below will make it more obvious to understand: VisualUpdateType | decimal | binary -- | -- | -- Data | 2 | 000000010 Resize | 4 | 000000100 ViewMode | 8 | 000001000 Style | 16 | 000010000 ResizeEnd | 32 | 000100000 FormattingSubSelectionChange | 64 | 001000000 FormatModeChange | 128 | 010000000 FilterOptionsChange | 256 | 100000000 All | 510 | 111111110
So, using binary system, it is possible to define what flags/types was sent just from one single number. Example: combination of "Resize" and "ResizeEnd" will be a "100100" in binary and 36 in decimal. That's why it is good idea to use bitwise operators to work with `VisualUpdateType`, for example like in a couple of comments above.
dm-p commented 1 year ago

Hi @Demonkratiy, and thanks very much for clarifying.

So, as I now understand it, any time you add a new VisualUpdateType to the visual host, we're going to get an additional bit's worth of information, and therefore the decimal value of our type (most notably for All based on previous issue reports will change accordingly)?

However, I'm unsure if I was ever able to know this without this issue coming to light. Has it ever been documented anywhere that this is how they should be used? I've looked around and this doesn't seem to be a common use case for enums in TS (but it is definitely very useful).

From testing, the above example doesn't evaluate to true for me, so wasn't working. Adding an equality check does though (which is as expected now the logic is understood):

if (powerbi.VisualUpdateType.All === (options.type & powerbi.VisualUpdateType.All)) { /* ... */ };
if (powerbi.VisualUpdateType.Data === (options.type & powerbi.VisualUpdateType.Data))  { /* ... */ };
if (powerbi.VisualUpdateType.Resize === (options.type & powerbi.VisualUpdateType.Resize))  { /* ... */ };
if (powerbi.VisualUpdateType.ResizeEnd === (options.type & powerbi.VisualUpdateType.ResizeEnd))  { /* ... */ };
if (powerbi.VisualUpdateType.Style === (options.type & powerbi.VisualUpdateType.Style))  { /* ... */ };
if (powerbi.VisualUpdateType.ViewMode === (options.type & powerbi.VisualUpdateType.ViewMode))  { /* ... */ };
etc.

If nothing else, to prevent similar frustrations for newcomers (and some of us that think we know more than we actually do!) it would be great if the note about type being bitwise could make it into the API doc as comments, and also the developer doc site. But, given that this now clarifies how we should be inspecting the VisualUpdateType, I'm going to close.

Thanks again,

Daniel