Replace glslify with brfs #236
Conversation
Improve DX by enabling hot reload of vertex and fragments shaders. Ease usage outside of Browserify. Related to #231.
| @@ -13,5 +13,5 @@ TODO | |||
| _docs | |||
| examples/winds/data/*.html | |||
| node_modules | |||
| npm-debug.log | |||
| *.log* | |||
There was a problem hiding this comment.
Nit: Maybe this wildcard is a little bit too general?
There was a problem hiding this comment.
Hum yeah npm-debug.log sometimes gets suffixed with numbers, maybe *.log.* would be better.
|
|
||
| const glslify = require('glslify'); | ||
| import {Layer, assembleShaders} from '../../../../../../index'; |
There was a problem hiding this comment.
Nit: Why are we moving this import line down? I think we should follow a logical ordering when including files (e.g. our own code first, then our own modules, then standard modules).
There was a problem hiding this comment.
I find it interesting to separate vendors modules from project source files, this way we know for sure what belongs is a dependency or not. Might be obvious right now because of the relative url but I would like to use absolute one in a near future, so I'm just laying to fondations.
| import {Model, Program, Geometry, glGetDebugInfo} from 'luma.gl'; | ||
|
|
||
| const glslify = require('glslify'); | ||
| import {Layer, assembleShaders} from 'deck.gl'; |
There was a problem hiding this comment.
Nit: Maybe have deck.gl before luma.gl? Higher level to lower level import order.
There was a problem hiding this comment.
luma.gl being a dependency of deck.gl, I find it more logic to put luma first.
|
|
||
| const glslify = require('glslify'); | ||
| import {Layer} from '../../../lib'; |
There was a problem hiding this comment.
These are imported differently in other layers (using single import line), let's be consistent.
There was a problem hiding this comment.
Any example? Don't see it.
There was a problem hiding this comment.
The second file in this diff...
| import {join} from 'path'; | ||
| import earcut from 'earcut'; | ||
|
|
||
| import {Layer} from '../../../lib'; |
There was a problem hiding this comment.
Ditto, order and combine imports
| import {GL, Model, Geometry} from 'luma.gl'; | ||
| import {join} from 'path'; | ||
| import {readFileSync} from 'fs'; | ||
|
|
||
| import {Layer} from '../../../lib'; |
There was a problem hiding this comment.
order and combine imports
| import {join} from 'path'; | ||
| import earcut from 'earcut'; | ||
|
|
||
| import {Layer} from '../../../lib'; |
|
Wouldn't an alternate approach be to prebuild and tell webpack users to rely on the prebuilt version? That seems to be common (mapbox-gl-js does this). Unrelated, but if glslify is kept in then you can use this https://github.com/hughsk/glslify-optimize |
|
@contra The issue was that even if relying on the builded and minified version of I can look into integrating the |
balthazar
force-pushed
the
deprecate-glslify
branch
2 times, most recently
from
516e5ef to
fff5fbb
Compare
balthazar
force-pushed
the
deprecate-glslify
branch
from
fff5fbb to
1d652ea
Compare
Improve DX by enabling hot reload of vertex and fragments shaders.
Ease usage outside of Browserify.