search

nglviewer / ngl

WebGL protein viewer
http://nglviewer.org/ngl/
MIT License
657 stars 168 forks source link

#1010 Colormaker-constructor #1011

Closed ppillot closed 5 months ago

ppillot commented 5 months ago

This PR fixes issue #1010 where the syntax for registering a ColorMaker does not generate valid JS.

After bissection, I've found out that this issue has been introduced with the recent changes to the build process (PR #993). It's possible that the transpilation + bundling results in invalid JS code (?). The fix was already done in the update-three,js ongoing branch, and had been cherry-picked from there.

ppillot commented 5 months ago

@fredludlow I've made some further tests and, noticed that one of my projects, it breaks the Typescrip checks. If I ignore the checks, the code works well though.

image

I'll investigate further.

ppillot commented 5 months ago

Issue resolved: it was a missing typescript definition for the argument of the addScheme method. I've checked on a personal project and in every "Color" example of the NGL webapp.

ppillot commented 5 months ago

That is a little strange, and surprising why the existing classical (explicit prototype) method fails, but switching to the class syntax reads more clearly anyway.

I am wondering if that's not an issue with terser, which can turn function declarations in more compact arrow functions. It's supposed to avoid doing so when there a this in the function body...