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

api.version.compare('>=', '0.5.31') no longer works #2276

Closed
zsviczian opened this issue Mar 21, 2024 · 7 comments · Fixed by #2278
Closed

api.version.compare('>=', '0.5.31') no longer works #2276

zsviczian opened this issue Mar 21, 2024 · 7 comments · Fixed by #2278
Labels
bug Something isn't working.

Comments

@zsviczian
Copy link

What happened?

ExcaliBrain uses const DVAPI = getAPI(); to access the DataView API.
With the latest release checking for API version with DVAPI.version.compare('>=', '0.5.31') returns an error.

DQL

No response

JS

No response

Dataview Version

0.5.65

Obsidian Version

1.5.11

OS

Windows

@GottZ
Copy link
Contributor

GottZ commented Mar 23, 2024

oh.. I've noticed this too and thought it's caused by code on their end.. I'll take a look again!
thanks for reporting.

@GottZ
Copy link
Contributor

GottZ commented Mar 23, 2024

aparently we have a race condition here..
image
you can clearly see the breakpoint that hit first, so for some reason, the version compare excalibrain is asking for, is called before dataviewjs is initialized.

edit:

oooh.. I see why:

image

I got to find the cause of this.. brb..

@GottZ
Copy link
Contributor

GottZ commented Mar 23, 2024

there ye go. that pull request should fix it.
@blacksmithgu after this issue is resolved we might need another public release, as this is actually breaking other plugins for an unknown reason.

I really have no clue how this code worked for years and is now breaking due to a change in execution order.
to me, this looks like a change in v8, which we can't do anything about.

@GottZ
Copy link
Contributor

GottZ commented Mar 23, 2024

I think #2265 caused this.

@blacksmithgu
Copy link
Owner

That seems most likely but very strange that an ES version bump would cause this, since it seems like a breaking change.

@GottZ
Copy link
Contributor

GottZ commented Mar 24, 2024

That seems most likely but very strange that an ES version bump would cause this, since it seems like a breaking change.

I assume it was poly filling some part of it before.
I'll compare the compiled code later for a in-depth analysis.

right now, the js that is generated pretty much 100% reflects the typescript source.
if it was transpiling even the slightest, it could lead to other behavior.

I also decreased the compatibility boundary.
you could compile modern js to work in Internet Explorer but in this case, I made the lowest boundary also es2022

@GottZ
Copy link
Contributor

GottZ commented Mar 24, 2024

ok.. found the cause, and as expected, it's precisely due to a compiler change:

this is before the ES2022 change:
image

this is after the ES2022 change:
image

the difference is simple.
we were on ES2018 before.
since ES2019, class members can be defined outside the constructor, thus the compiler moved them all out of the constructor.

anything outside the constructor obviously runs before the constructor is ran, making it a static default of the class. (essentially adding it to the prototype)

to reverse this behavior, it would either have to be inlined into the constructor or kept the way I changed it, so it refers to the current instance, and makes it a true, static member of the class:
image

in essence, for the javascript engine the 2022 spec is causing less in-memory fragmentation, thus the plugin now has a smaller memory footprint than before.
I can go in dept about engine optimizations now but just trust me on that. I know v8 pretty well.

now comparing all of it with the typescript source,
it makes sense, why it now is as it is:
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants