Latest master npm build broken because implicit THREE global is used

squarefeet / ShaderParticleEngine

A GLSL-heavy particle engine for THREE.js. Originally based on Stemkoski's great particle engine (see README)

http://squarefeet.github.io/ShaderParticleEngine/

MIT License

853 stars 115 forks source link

Latest master npm build broken because implicit THREE global is used #95

Open AndrewRayCode opened 8 years ago

AndrewRayCode commented 8 years ago

This is different from #94

Using the latest build on master:

require('shader-particle-engine-hotfix')
    ReferenceError: THREE is not defined

Currently the compiled main distributed source code uses an implied global THREE.

In the source code I think three should be required with commonjs syntax and included in package.json as a peerDependency. Or you could go the route taken by the OrbitControls npm project and make the user pass in THREE to some top level wrapper.

squarefeet commented 8 years ago

This is fantastic, thanks Andrew!

A few points, though:

I've been considering refactoring this to make use of ES6 (mainly classes, import declarations, etc.), which would help somewhat in fixing up the currently quite haphazard NPM implementation. ES6 support is now (IMO) good enough to warrant having a pure ES6 build, alongside a Babel-compiled ES5 version.
I'm conscious of ensuring that those people who currently use the library without NPM aren't forced into using it as such, that it could still be used as a drop-in script.

I'll have a proper look over these PRs, but from the brief look I've just had, it seems like an interesting start :)

AndrewRayCode commented 8 years ago

It would be a very minor upgrade to switch from this to ES6 modules, you'd just have to change the require() calls in here to import/export.

AndrewRayCode commented 8 years ago

Also whatever builds the distributed bundle that's put into build/ will have the correct configuration to sort out the require() calls. npm users will use the normal entry flow and others will use the built file, which most js build tools now know how to work with commonjs

AndrewRayCode commented 8 years ago

Also I see you pushed 1.0.5 to npm, I would suggest not pushing to npm until this problem is fixed, because this package still fails with the error in the original ticket

Friksel commented 7 years ago

Having the same problem here using the NPM package 1.0.5 or 1.0.6. It's now a year later. Would it be possible to push a version to NPM without the need for the THREE (and SPE?) globals? It would be great to be able to use this nice thing through NPM!