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.

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

build.js w/ closure, npm run build/watch/serve/watch-serve #8366

Closed
wants to merge 6 commits into from

Conversation

bhouston
Copy link
Contributor

This replaces the python-based build script with the build.js script. I've upgraded the build.js script to use the google closure compiler via the npm mobule https://www.npmjs.com/package/google-closure-compiler This means that I have removed build.py from the project as well as the Google Closure Compiler binary (as it is now installed via npm.)

I have also merged into this branch @mattdesl's npm build and watch commands.

I've also added a serve command that starts up a web server.

And lastly I've added the "npm run watch-serve" which starts the auto-build and web-server together. Thus you can develop and it will automatically build and serve your files. I love this mode for speeding up my development workflow.

@mrdoob
Copy link
Owner

mrdoob commented Mar 17, 2016

This means that I have removed build.py from the project as well as the Google Closure Compiler binary (as it is now installed via npm.)

Would it be possible to keep the binary for now? One of the things I try with the library is to have it ready to go once downloaded (without having to install additional dependencies).

@bhouston
Copy link
Contributor Author

I can undo it.

The other issue with this PR is that we often build three.min.js without minimization while at the same time not updating three.js -- e.g. what build_debug.bat does. This is actually sort of confusing.

I wonder if we should have the examples load three.js to make them easier to debug? Then we wouldn't sort of promote using three.min.js as not actually minified.

Also maybe we could have the build command that would build three.js and if you add a "--minify" it also creates three.min.js. Thus it would either just build three.js or both. The benefit of building both if you are using minification is that one can reference three.js from three.min.js as the unminified source via source maps. If you just build three.min.js without an unminified version somewhere, there is nothing to reference for the source maps. It actually doesn't cost much extra to build both as compared to just building three.min.js, as it is the minification that is actually the slow part.

@WestLangley
Copy link
Collaborator

I wonder if we should have the examples load three.js to make them easier to debug?

Many new users on SO use three.min.js, and are clueless when they get errors. In fact, many don't know there is a non-minified version. I am inclined to agree with this suggestion.

@mrdoob
Copy link
Owner

mrdoob commented Mar 17, 2016

I think I agree too 😊

js: tempSourceFilename,
externs: args.externs,
jscomp_off: [ 'checkTypes', 'globalThis' ],
compilation_level: 'SIMPLE',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

your tabs vs. whitespace seems off here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, his whitespace is fine, I guess. That file contained quirky spaces and I removed them too - we'll have fun merging :).

@abelnation
Copy link
Contributor

abelnation commented May 6, 2016

I just took a crack at trying to update the build.js script in #8822. I've yet to be completely successful, and the build.js script definitely seems more complex/fragile than a simple tool like closure might possibly be.

I do think the concat method of building the non-minified source is nice, in that the resulting file is easy to inspect, but it seems more prone to weird issues when any of the code is preprocessed or modified.

@bhouston
Copy link
Contributor Author

bhouston commented May 6, 2016

This PR does do the building correctly though. So just copy code from it. :)

@abelnation
Copy link
Contributor

abelnation commented May 6, 2016

Is it possible to update the build.js script to not require being run from the build dir?

It seems more ideal and flexible if the build script could be run from any path. The script can detect it's own path via __dirname and then resolve all other paths relative to that.

see: https://github.com/abelnation/three.js/blob/improve-npm-build/utils/build/build.js#L8
(there are other, unrelated changes in that branch, but i'm linking just to show the relative path resolving)

@mattdesl
Copy link
Contributor

mattdesl commented May 6, 2016

If TheeeJS just used a single package json with necessary scripts/deps in root, you could npm like a typical node package, and then "npm run build" from any folder within threejs.

Sent from my iPhone

On May 6, 2016, at 12:24 PM, Abel Allison notifications@github.com wrote:

Is it possible to update the build.js script to not require being run from the build dir?

It seems more ideal and flexible if the build script could be run from any path. The script can detect it's own path via __dirname and then resolve all other paths relative to that.

see: https://github.com/abelnation/three.js/blob/improve-npm-build/utils/build/build.js#L8


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a note that closure is under the Apache2 license and a link to that license somewhere? I am not a lawyer, but I guess we'd have to add it somewhere in case not.

@bhouston bhouston mentioned this pull request Jul 11, 2016
@zz85
Copy link
Contributor

zz85 commented Sep 2, 2016

seems like this can be closed now

@bhouston bhouston closed this Sep 2, 2016
@Jozain Jozain deleted the node-closure-build branch November 28, 2016 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants