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.

Sign up for GitHub

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

Jump to bottom

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

Open
AndrewRayCode opened this issue Mar 16, 2016 · 5 comments
Open

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

AndrewRayCode opened this issue Mar 16, 2016 · 5 comments

Comments

@AndrewRayCode
Copy link
Contributor

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.
AndrewRayCode added a commit to AndrewRayCode/ShaderParticleEngine that referenced this issue Mar 16, 2016
@AndrewRayCode
Replacing jshint with eslint
aecfccb 
This is a first step to modernizing the codebase and to make it work
with npm. Ticket squarefeet#95 currently tracks the npm build process.
AndrewRayCode added a commit to AndrewRayCode/ShaderParticleEngine that referenced this issue Mar 16, 2016
@AndrewRayCode
Removing implicit use of THREE and SPE in source
7deb182 
Related to issue squarefeet#95, the source code of this project relies on the use
of the global SPE variable and the global THREE variable. This makes it
hard to track what files use what dependencies. Plus the javascript
community has moved to commonjs or es6 module syntax for good reason. It
makes it easier to work on projects. Relying on the order of a file
included in an array of strings is not ideal for dependency management.

This also removes the SPE namespace from files which felt redundant, and
also mirrors the elimination of the SPE namespace attachments used
inside those files.

This namespaces all of the objects in a central place src/index.js and
exports that as main from package.json
AndrewRayCode added a commit to AndrewRayCode/ShaderParticleEngine that referenced this issue Mar 16, 2016
@AndrewRayCode
Removing implicit use of THREE and SPE in source
4c4d320 
Related to issue squarefeet#95, the source code of this project relies on the use
of the global SPE variable and the global THREE variable. This makes it
hard to track what files use what dependencies. Plus the javascript
community has moved to commonjs or es6 module syntax for good reason. It
makes it easier to work on projects. Relying on the order of a file
included in an array of strings is not ideal for dependency management.

This also removes the SPE namespace from files which felt redundant, and
also mirrors the elimination of the SPE namespace attachments used
inside those files.

This namespaces all of the objects in a central place src/index.js and
exports that as main from package.json
AndrewRayCode added a commit to AndrewRayCode/ShaderParticleEngine that referenced this issue Mar 16, 2016
@AndrewRayCode
Removing implicit use of THREE and SPE in source
d2b13a2 
Related to issue squarefeet#95, the source code of this project relies on the use
of the global SPE variable and the global THREE variable. This makes it
hard to track what files use what dependencies. Plus the javascript
community has moved to commonjs or es6 module syntax for good reason. It
makes it easier to work on projects. Relying on the order of a file
included in an array of strings is not ideal for dependency management.

This also removes the SPE namespace from files which felt redundant, and
also mirrors the elimination of the SPE namespace attachments used
inside those files.

This namespaces all of the objects in a central place src/index.js and
exports that as main from package.json
AndrewRayCode added a commit to AndrewRayCode/ShaderParticleEngine that referenced this issue Mar 16, 2016
@AndrewRayCode
Removing implicit use of THREE and SPE in source
9b5e0f0 
Related to issue squarefeet#95, the source code of this project relies on the use
of the global SPE variable and the global THREE variable. This makes it
hard to track what files use what dependencies. Plus the javascript
community has moved to commonjs or es6 module syntax for good reason. It
makes it easier to work on projects. Relying on the order of a file
included in an array of strings is not ideal for dependency management.

This also removes the SPE namespace from files which felt redundant, and
also mirrors the elimination of the SPE namespace attachments used
inside those files.

This namespaces all of the objects in a central place src/index.js and
exports that as main from package.json
AndrewRayCode added a commit to AndrewRayCode/ShaderParticleEngine that referenced this issue Mar 16, 2016
@AndrewRayCode
Removing implicit use of THREE and SPE in source
0cd6780 
Related to issue squarefeet#95, the source code of this project relies on the use
of the global SPE variable and the global THREE variable. This makes it
hard to track what files use what dependencies. Plus the javascript
community has moved to commonjs or es6 module syntax for good reason. It
makes it easier to work on projects. Relying on the order of a file
included in an array of strings is not ideal for dependency management.

This also removes the SPE namespace from files which felt redundant, and
also mirrors the elimination of the SPE namespace attachments used
inside those files.

This namespaces all of the objects in a central place src/index.js and
exports that as main from package.json
@squarefeet
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link

Friksel commented Jul 24, 2017 •
edited
Loading

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@AndrewRayCode @squarefeet @Friksel