Make JS output Node or web only #6542
Comments
|
What about a more modern solution such as Universal JavaScript as described in [1] and [2]? [1] https://hackernoon.com/how-to-make-universal-javascript-applications-part-1-90e9032bc471 |
|
The more universal, the better! The problem here is that the node path needs the |
|
Why not make a universal module for this one? If we really need separate code for disk vs web I would favor generating separate code for this part only. |
|
You should be able to clean up a lot of this code through using a minifier, for example see https://github.com/mishoo/UglifyJS2#conditional-compilation-api In general though, it would probably be better for this to both be controllable through a compiler time setting. A PR would be welcome. @brodybits I'm not really sure why you bring up universal JS as something new, as that's what this is already doing: abstracting over browser differences so that one library can be run anywhere. |
|
I found an old issue about this: #1140 the conclusion there was somewhat negative, but I do think this is worth doing even if it is only for a few K of savings. PR would be very welcome. |
|
Re-reading this, I apologize for overlooking point 2, that it breaks bundlers. We should fix that at high priority, I'm working on a PR now. |
|
It doesn't necessarily break bundlers, you may just have to change some settings. For Browserify that means using |
|
True, it doesn’t break bundlers, but it needs some re-configuration that is easily avoided imo (see this webpack issue). It also adds a dependency to the module during static analysis that is not actually a real dependency. In general it seems more idiomatic (danger, personal opinion) to only emit code for one platform if it can’t be written in a universal way and let the developer choose which platform to target. For backwards-compatibility’s sake the current behavior could stay the default and a new flag could be added to explicitly opt into emitting code for only one platform. |
|
I see, thanks @curiousdannii. Perhaps less urgent than I thought, then. Still, it seems nice to have this. Simple implementation up at #6565, comments very welcome there. |
) By default we still emit code that can run in node, web, shell, etc., but with the option the user can fix only one to be emitted. This also sets ENVIRONMENT to "web" if the user specifies an HTML output (emcc [..] -o code.html). In that case the indication is clear that the code should only run on the web. This is technically a breaking change though, as people may have tried to run the .js file outside of the web, and that may have worked in some cases. To prevent confusion, when ASSERTIONS are on we show an error if we hit a problem, and also in debug logging we mention doing HTML output setting the environment that way. The benefit to doing this is that it gets all the benefits of web-only emitting in a natural way for the main use case. For more details on the benefits, see #6542 #1140, but basically * When targeting only web, we don't emit node.js support code which includes require() operations that confuse bundlers. * We emit less unneeded code.
|
Implemented by that PR. |
- this is what I was having trouble with when I first created this repo - c.f. kripken/ammo.js#109 (comment) , webpack/webpack#7352 , and emscripten-core/emscripten#6542 - can't just upgrade to a new ammo as that might very well break physijs, so just make the Node environment check false - in this case I changed `var qa="object"===typeof process,` to `var qa=false,` in the minified code - could also remove the `if(qa){`... parts now, though that'll probably get re-minified too and nbd
- this is what I was having trouble with when I first created this repo - c.f. kripken/ammo.js#109 (comment) , webpack/webpack#7352 , and emscripten-core/emscripten#6542 - can't just upgrade to a new ammo as that might very well break physijs, so just make the Node environment check false - in this case I changed `var qa="object"===typeof process,` to `var qa=false,` in the minified code - and also remove thed `if(qa){`...`}` parts - this was the part that had `require` statements, so now webpack etc should be able to parse it without a problem - there was a `require("fs")` and a `require("path")` in there - alternatively, could build and replace stuff with webpack, but I'd need to provide that anyway for auto-config - downside is it might no longer work on Node, but not the target audience so w/e
- use physijs-webpack, a fork I created a few years ago and finally
took some time to debug and get working
- use via github since someone took the NPM name a few years ago and
referenced my repo in it even though it never worked until today...
- maybe will update once I get that resolved
- now we don't have to vendor in all those scripts anymore and don't
need to manually configure Physijs either!
- remove the vendored physijs and three scripts
- had to debug and modify ammo to get that version working with
webpack / bundlers in general
- newer versions of Emscripten can target specific envs
- c.f. kripken/ammo.js#109 (comment) ,
webpack/webpack#7352 ,
and emscripten-core/emscripten#6542
- and then made the worker config more flexible
- add worker-loader as a devDep per physijs-webpack instructions
- fix publicPath location as this is now actually used
- the worker is loaded based on the publicPath
- add three-window-resize and three-trackballcontrols as deps as well
- since three's examples/js/ folder doesn't quite work with bundlers
- c.f. mrdoob/three.js#9562
- maybe if I made some modifications, updated to newer Three
revision, and used imports-loader it might work 🤷 TBD
- upgrade physijs to latest and Three from r60 to r73
- latest physijs uses r73, so remain consistent
- also physijs-webpack has a peerDep for that specific version
- [email protected] also requires r73 as dep
- had to refactor a bit due to upgrade
- WebGLRenderer() -> WebGLRenderer({alpha: true})
- the canvas now defaults to black without this, which was extremely
disorienting
- shadowMapEnabled -> shadowMap.enabled
- CubeGeometry -> BoxGeometry
- quaternion._euler -> rotation
- I probably could've just used this before, couldn't have I...?
.
.
- TODO: improve webpack perf (CPU + Memory) and build speed
- this change slows down initial build times quite a bit (~20s), since
Three, Physijs, and Ammo are all parsed by Webpack now
- will want to update webpack to get a dev-server (wepback-serve)
running
- webpack itself is faster in later versions as well
- and perhaps add HardSource for caching otherwise or split out
vendored stuff into a DLL
- probably can't update until decaffeinate'd since I believe the
loaders used here are no longer maintained :/
- and then would need to figure out the literate programming
and its sourcemaps
- will want to output a separate vendor bundle per best practice
- may also want to output HTML via webpack too while at it
- the templates don't do any templating so would be nbd
- and would allow for hashing and therefore cache-busting
- no need to manually clean cache then
- and prod build / uglification is even _slower_ (~80s)
- may want to exclude some files from Uglify
- i.e. Ammo and use three.min so it can be excluded too
Currently, the JS glue code output by Emscripten targets both Node and Web and does some detection what environment it is running in.
This is problematic for 2 reasons:
require()directives that can’t be resolved.I think a good solution would be to allow users to specify which environment they are targeting and only emit the code for that specific target.
The text was updated successfully, but these errors were encountered: