Render merge - Githubissues

dasmoth / dalliance

Interactive web-based genome browser.

http://www.biodalliance.org/

BSD 2-Clause "Simplified" License

226 stars 68 forks source link

Render merge #200

Closed chfi closed 7 years ago

chfi commented 7 years ago

Adds support for ES6 to the build process, as well as the new rendering system and modules, and the render tests.

pjotrp commented 7 years ago

@dasmoth can you comment on this PR?

dasmoth commented 7 years ago

@chfi: Many thanks for putting this together

@pjotrp: Sorry I've been out of the loop for the last few days. Should get a chance to look this over at the weekend.

dasmoth commented 7 years ago

Sorry it's taken me a while to get to this -- given the relatively core nature of the changes, I'm keen to understand everything that's going on here.

A couple of quick observations:

I can't currently get test/render-test.html to work on my system (error is "Cannot read property 'renderTier' of undefined"). Is there some special invocation I'm missing that's needed for this to work.
The existing tests (conventionally launched with "karma start" or similar) are no longer running due to hitting some uncompiled ES6 -- is this the expected result?
The new "gulp lint-es6" target is reporting a long list of errors -- again, are these anticipated?

chfi commented 7 years ago

All those errors should be fixed now! However, now I found another problem with the new renderer, will check it out.

dasmoth commented 7 years ago

Thanks!

chfi commented 7 years ago

Now all tests are passing.

dasmoth commented 7 years ago

Brilliant, thanks @chfi. All working nicely now, and will try to get it merged at the weekend.

What's your view on removing the old renderer path once this is fully merged? I'm a little bit uneasy about the redundant code, and while the renderer stuff doesn't change often is does get tweaked from time to time so keeping them in sync will be a pain.

On the other hand, I think the testing offered by comparing the resulting scenegraphs is kind-of valuable.

One option that seems to keep the spirit of your test suite but allows removal of the old renderer would be to serialize the expected scenegraph (probably as JSON) then compare against that. In cases where someone's actively working on the renderer infrastructure, they can check the changes to the scenegraph manually, and commit an updated reference JSON once they're happy. Does this sound reasonable?

chfi commented 7 years ago

The only problem I have with removing the old renderer is that I'm still not entirely confident in the tests -- in fact, there have been a couple of bugs that they didn't catch. That's because the test data is pretty limited; it doesn't catch problems that only show up in some scales, for example.

I think your idea with serializing the scenegraph would solve that, since it'd allow for using both the test data as well as real data, which should catch whatever problems remain. It'd also make the testing process saner. I'm all for it!

dasmoth commented 7 years ago

Merged now.

Given the size of the changes, I've created a pre_render_merge branch just in case...

I've made a couple of tweaks to:

Babel-compile all the JS code, not just the new "*.es6" modules -- having taken the step of relying on an ES6 transpiler, we might as well use it everywhere. [At some point, it may make sense to rename the .es6 files but I haven't done that yet].
Port a couple of recent render changes into the new framework.

@chfi: thanks very much for all your work on this.

I'll open another issue relating to the removal of the old rendering paths.