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

:advanced for webpack #24

Closed
thheller opened this issue May 22, 2017 · 30 comments
Closed

:advanced for webpack #24

thheller opened this issue May 22, 2017 · 30 comments

Comments

@thheller
Copy link
Owner

thheller commented May 22, 2017

The plan is to support :advanced for webpack by abusing the Closure JSModule support but instead of grouping many files into one JSModule each file gets its own module. Coupled with RescopeGlobalSymbols that produces something that can be understood by webpack while still being reasonably small and DCE'd.

JS side would remain as require("shadow-cljs/demo.foo").foo() but the CLJS side would need to add the :export meta to anything that should be accessible from the outside, ie. (defn ^:export foo [] ..).

webpack could then do the chunking as normal. The :browser style module definition could also be used so we could keep the grouping and expose less files to webpack. API remains the same.

the output looks something like this:

var window = global;
var $ = require("./cljs_env");
$.module = module;
require("./cljs.core.js");
$.Ie = new $.T("code-split.a", "a", "code-split.a/a", 1094652911);
window.console.log($.Ie, $.module);
$.module.exports = {
  foo: function() {
    return "foo";
  }
}

$ is the the Closure "global" and everything is rescoped to it. So basically instead of having cljs.core.assoc you'd have $.cljs.core.assoc which then is shortened to something like $.x.

var window = global; is needed because the Closure Compiler assumes that is the "true" global. Would need a patch to Closure as it is hard-coded. Faking its existence works though.

$.module = module; is required since closure will rewrite EVERYTHING that wants to use a global variable to window, so js/module becomes window.module not module. Again that would need a Closure patch to add the node.js implicit vars to their set of globals.

See: https://github.com/google/closure-compiler/blob/master/src/com/google/javascript/jscomp/RescopeGlobalSymbols.java#L57-L80

I can't figure out why this doesn't work though? Am I just missing something obvious?

$.module.exports = {
  foo: function() {
    return "foo";
  }
}

@jiyinyiyong any ideas? if I change $.module.exports to module.exports it works but I can't yet make the closure compiler emit that.

Edit: https://github.com/thheller/shadow-cljs/blob/a2200a3ad6eae4c2e579be306a367ab29bf53ecd/src/test/code_split/a.cljs this file produced the above output.

@thheller
Copy link
Owner Author

Nevermind I figured it out. The require("./cljs.core.js"); set $.module itself so it was exporting into the wrong module.

@thheller
Copy link
Owner Author

No release yet but shadow-cljs is now able to run the CLJS part ofnpm builds though Closure and produce something that webpack can use. So the only the CLJS code is processed by Closure but not the normal JS.

:simple works and :advanced still needs a new CLJS release. CLJS-2040 was applied but not released yet.

There are some other :externs related things I need to figure out. It still renames a few things it shouldn't rename.

@thheller
Copy link
Owner Author

It will also require a tweak in the webpack config because UglifyJS (when running webpack -p ) will mess up the :advanced output. It must not process the compile CLJS code. I think there is a exclude option, didn't test that part.

@thheller
Copy link
Owner Author

I just pushed a new release that should enable :advanced for npm builds.

The usual :advanced caveats apply which means :externs are required. In addition to that only things that are exported via (defn ^:export foo ...) will remain callable from JS. Everything that is not exported will be removed since Closure will treat it as dead code.

This is very experimental and is probably going to need some adjustments.

I will try to create a guide for this but apart from providing the externs the only change required should be running shadow-cljs --release instead of shadow-cljs --once (or dev).

@thheller
Copy link
Owner Author

Quick example: https://github.com/thheller/npm-module-example

Important things:
https://github.com/thheller/npm-module-example/blob/master/shadow-cljs.edn

When a build with the :id :npm is found it will be used over the configuration in package.json. I don't like having 2 places where you can put configuration so all build related configuration is now in the shadow-cljs.edn config file. dependencies and source-paths will remain in package.json for now. output-dir still works as well but will probably be removed at some point.

https://github.com/thheller/npm-module-example/blob/master/externs.js
The externs.js are the minimal externs required to make everything work for this build. You can run shadow-cljs --check and Closure will warn about undefined properties. These must then be defined in externs or they will be renamed.

The demo.foo ns must export the hello fn since Closure doesn't know that index.js will be calling it.

Its all very manual still and I hope to automate some of it in the future.

@tiye
Copy link
Collaborator

tiye commented May 27, 2017

In your example there's a shadow-cljs.edn file, while I use package.json to add configs with output-dir. I found output-dir not working after I added shadow-cljs.edn.

@thheller
Copy link
Owner Author

What does your config look like then?

[{:id :npm
  :target :npm-module
  :output-dir "compiled"
  :compiler-options
  {:externs ["externs.js"]}}]

This should work?

@tiye
Copy link
Collaborator

tiye commented May 27, 2017

Yeah, it's working. And the result is surprising:

=>> du -ah dist/*.js
4.0K	dist/main.7682f952.js
148K	dist/vendor.0139d23f.js

Can I write all the configs in that EDN file?

@thheller
Copy link
Owner Author

dependencies and source-paths will remain in package.json for now

That is because these must be available before the JVM is launched but I'll eventually move to a central config file. For now I didn't want to mix build related things with classpath things since classpath is also covered by lein.

Why is the result surprising? Looks about right for an :advanced compiled build.

@tiye
Copy link
Collaborator

tiye commented May 27, 2017

Previously I was using Boot, and the result was 700k+ even in :advancedmode. Probably a problem in my Boot configs.

@thheller
Copy link
Owner Author

Ah ok. boot should come to about the same size, shouldn't make much of a difference. Maybe you were testing with :pseudo-names true or so? That would increase the size significantly.

@tiye
Copy link
Collaborator

tiye commented May 27, 2017

Checked out my old commit and got 144k. Anyway confirmed Boot is fine.

The options was right https://github.com/mvc-works/stack-workflow/blob/bdb24382dd3a4a0b20c30720f5e6ae97d3075291/build.boot#L27 . It must be another mistake.

@thheller
Copy link
Owner Author

Good to know. Can you compare the gzip'd sizes? wondering much of a difference the webpack boilerplate code makes in the end.

@tiye
Copy link
Collaborator

tiye commented May 27, 2017

Almost the same when I compress them with gzip -c x.js > x.js.gz.

With Boot:

=>> du -ah main.js*
144K	main.js
 36K	main.js.gz

With shadow-cljs and Webpack

=>> du -ah *.js*
 12K	main.861c0331.js
4.0K	main.861c0331.js.gz
4.0K	manifest.json
140K	vendor.4780ee50.js
 36K	vendor.4780ee50.js.gz

@thheller
Copy link
Owner Author

I seem to be incapable of correctly configuring webpack in a way that would correctly mirror my current CLJS setup with code-splitting but besides that the :advanced support seems to be working correctly.

Closing this until someone says otherwise.

tiye referenced this issue in minimal-xyz/minimal-shadow-cljs-commonjs Jun 2, 2017
@tiye
Copy link
Collaborator

tiye commented Jun 12, 2017

I think we need to move some of this configs to Wiki. Already wanted to read it for several times but had to trace back here.

@thheller
Copy link
Owner Author

Pretty busy with other stuff at the moment so I didn't get far with the docs. I want a full guide about taking a build from dev to release with :externs and everything but you could probably write a book about that.

Thanks for reminding me that docs are more important than new features. I'll try ...

@tiye
Copy link
Collaborator

tiye commented Jun 12, 2017

As a user, a full guide is less important to me actually. Even people offer me a guide, it could be long enough for me to ignore a big part of the details. And it's quite important for me that when I have a need, I can always google and find examples to copy configs from. Will try tomorrow.

@tiye
Copy link
Collaborator

tiye commented Jun 20, 2017

Reading about Webpack 3 scope hoisting this morning, i got an question, is it possible shadow-cljs compile optimized cljs code into 2 bundles and let Webpack see them as bundles? Currently shadow-cljs emits code in CommonJS and it does not benefit from this new feature.

https://medium.com/webpack/webpack-3-official-release-15fd2dd8f07b
https://github.com/webpack/webpack/tree/master/examples/scope-hoisting

@thheller
Copy link
Owner Author

Scope hoisting does nothing for us since Closure already assumes one global shared scope. You can already just concat Google Closure files together with absolutely no changes and it just works. There are no conflicts, you don't need import/export.

:npm-module just tries to make it look like a shared scope so webpack can understand it. The Closure Compiler already does a far far better job than webpack scope hoisting ever can.

I can add an additional :browser-library target similar to :node-library which pre-bundles all CLJS into one or more :modules each with :exports so you load require("shadow-cljs/bundle-a") and don't get individual namespaces but a bunch of them grouped together.

If you really care about performance/file size then use shadow-cljs as the "bundler" with :browser. It will provide better results than webpack. The only thing missing is getting npm dependencies bundled automatically so you don't need the manual "double bundle". I did a quick prototype of my "plan" here: https://github.com/thheller/shadow-cljs/tree/master/out/webpack-dll

I'm still working on the details since all this gets rather complicated if you factor in code splitting. Ideally I also want 100% interop so JS can import CLJS and vice versa but that will probably only work when we let the Closure Compiler optimize all JS.

The webpack support was always meant to support users that have a JS codebase and build setup configured and just want to add CLJS. If your primary language is CLJS we shouldn't need webpack in the end. There is still work to be done for this though.

@tiye
Copy link
Collaborator

tiye commented Jun 20, 2017

I would be fine with :browser mode, as long as I can do these works:

  • code splitting
  • long term caching
  • import and bundle CSS

It appear to me we always need Webpack for the CSS issue.

Yeah, it's far more complicated than I thought...

@thheller
Copy link
Owner Author

Code splitting (:modules) and long term caching (:module-hash-names true) work already, probably much better than webpack. I wrote a little about this over here: https://github.com/thheller/shadow-cljs/wiki/ClojureScript-for-the-browser#productionrelease-builds

I have scss support in my work project, completely without webpack. I moved it out of shadow-cljs for now since it was pretty specific to my setup. I need to think about it more to make something that can be useful for more people.

Lately I have been using shadow.markup which does CSS-in-CLJS. It works well but generic CSS support is still useful.

No, we won't always need webpack but it is useful for now.

@tiye
Copy link
Collaborator

tiye commented Jun 20, 2017

I found you are using:

[{"name":"main", "js-name":"main.md5hash.js", "depends-on":[], ...}
 {"name":"extra", "js-name":"extra.md5hash.js", "depends-on":["main"], ...}]

I would prefer an HashMap for manifests:

{
 "main.css": "main.a7a4c1d5.css",
  "main.css.map": "main.a7a4c1d5.css.map",
  "main.js": "main.a7a4c1d5.js",
  "main.js.map": "main.a7a4c1d5.js.map",
  "vendor.js": "vendor.1f6ff49f.js",
  "vendor.js.map": "vendor.1f6ff49f.js.map"
}

I think I should try that now. Anyways I was more familiar with Webpack.

@thheller
Copy link
Owner Author

It is structured that way for a reason (modules in dependency order, includes more than just name:js-name mapping).

It will probably never contain css info since that has nothing to do with loading JS. My stuff currently writes a css-manifest.json.

It is however all pure data so should be easy to transform it into any structure you want.

@tiye
Copy link
Collaborator

tiye commented Jun 20, 2017

Agreed.

@tiye
Copy link
Collaborator

tiye commented Jun 27, 2017

The filenames are too uglify.

main.B882157F6C570D9DDB7E9B861F7BB5DB.js

Can we change it to something like ths?

main.a7a4c1d5.js

@thheller
Copy link
Owner Author

Does that matter at all? Only a machine is reading that and I care more about not ever having conflicts? I don't think the shorter name is any prettier?

I can add an option if you really want to ...

@tiye
Copy link
Collaborator

tiye commented Jun 27, 2017

I can't say the shorter one is pretty... but when that filename is so long, it becomes harder to manage in some cases.
image

would be better in this way:
image

@thheller
Copy link
Owner Author

I think that goes away with trust? I rarely look at my release files.

I just looked and it currently has 151 files. 4 from current release, the rest from old versions. I keep all old versions in case someone (web crawlers) have old HTML and want to access old JS.

I always group my release files in an extra folder as well (eg. dist/js/, dist/css/, ...). You have that for fonts it seems.

Anyways, making this configurable is easy so I'll add it soon.

@tiye
Copy link
Collaborator

tiye commented Jun 27, 2017

When we use Webpack long term caching, we keep all file on CDN. But the dist/ on laptop is always cleaned before building process. Sometimes we check file folder for details, like figuring out some problems.

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

No branches or pull requests

2 participants