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(datasource/graphene) fix multicut annotation list not appearing in graph tab #706

Closed

Conversation

chrisj
Copy link
Contributor

@chrisj chrisj commented Jan 31, 2025

My testing has shown this feature was broken in 04ed8f9

My original implementation was a way to get around having to modify AnnotationLayerView by overriding get annotationStates() in the graphene derived class.

What I observe is that the private property _annotationStates is not shared when annotationStates is called from the base class and from the derived class, it needs to be initialized twice.

It makes sense it would break in this commit because useDefineForClassFields is very much related to base and derived classes but if I set useDefineForClassFields to false and "target" back to to "ES2017", it doesn't fix the issue. Do you have any idea why? I'd like to understand it better.

@jbms
Copy link
Collaborator

jbms commented Jan 31, 2025

Hi Chris, sorry for breaking this. That change was unfortunately pretty tricky to test because of the possibility of silent breakage. Evidently I missed some things.

I think what is happening is that inside the superclass constructor, the annotationStates getter is being called which initializes _annotationStates. However, immediately after the superclass constructor call, per how class fields work with useDefineForClassFields=true (which is also how they work when not using typescript), there is effectively a call to Object.defineProperty(this, "_annotationStates", { value: undefined }); which resets _annotationStates back to undefined.

The solution is to declare the class field using the syntax declare private _annotationStates: .. instead of private _annotationStates: .... declare means it gets stripped out during transpilation, which effectively restores the useDefineForClassFields=false behavior for that member.

As for why changing the option in tsconfig.json doesn't work, bundling now happens with rspack which uses SWC for transpilation and does not pay much, if any, attention to tsconfig.json. The options in tsconfig.json basically now just influence typechecking, which serves as a lint. Previously, transpilation used esbuild which read tsconfig.json and pretty faithfully matches tsc behavior. SWC does have a useDefineForClassFields option but its behavior is subtly different from that of tsc and esbuild, which meant it could not be relied upon. Ensuring consistent behavior across different bundlers was the motivation for changing that option in the first place.

@chrisj
Copy link
Contributor Author

chrisj commented Jan 31, 2025

@jbms thanks for the explanation, the tsconfig was confusing me but that makes sense now. I created a separate PR with your solution of using of define to fix the issue.

#707

I'll keep this one up for now just in case you prefer it.

@chrisj chrisj closed this Jan 31, 2025
@chrisj chrisj deleted the cj-multicut-annotation-list-fix branch January 31, 2025 21:20
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.

2 participants