HenrikJoreteg / moonboots

A set of conventions and tools for bundle and serving clientside apps with node.js
159 stars 20 forks source link

3.0.0 - Cannot make sourcemaps and minify work together #57

Open codeaholics opened 9 years ago

codeaholics commented 9 years ago

sourceMaps is set to true.

If minify is set to true, the JS is correctly minified, but there are no sourcemaps at the end.

If minify is set to false, the JS is bundled (browserify) but not minimised (as expected) and the sourcemaps ARE present.

It seems that setting minify to true somehow disables sourcemaps.

latentflip commented 9 years ago

It's not that setting minify to true disables sourcemaps, it's that the minifier we use (mishoo/UglifyJS2) doesn't output sourcemaps by default. We would need to pass sourcemap config through to uglify to make sourcemaps + minifying work together.

codeaholics commented 9 years ago

Ah. Is that easy to do or if not, could you provide me some pointers to take a look at it myself? It's a shame because the value of sourcemaps isn't that great with just the bundled code. They really come into their own when minifying.

latentflip commented 9 years ago

Shouldn't be very complex: figure out what the config option is for uglify, and where we call out to uglify from moonboots, and add the option if source maps is set in the moonboots config.

The bigger challenging question is what kind if source maps you want? If you want them concatenated to the end of your file then you've just lost all the benefit of minifying. If you want them served as separate files then we would need to add more config for that somewhere I believe; which will be a more complex discussion either here or in moonboots-Hapi/express; or both.

Philip Roberts

On 21 Oct 2014, at 14:58, Danny Yates notifications@github.com wrote:

Ah. Is that easy to do or if not, could you provide me some pointers to take a look at it myself? It's a shame because the value of sourcemaps isn't that great with just the bundled code. They really come into their own when minifying.

— Reply to this email directly or view it on GitHub.

codeaholics commented 9 years ago

The minifyify plugin for browserify (https://github.com/ben-ng/minifyify) seems to get this right. It generates 2 files - a minified bundle and a separate map file which includes the raw source so you don't have to separately serve it. Perhaps there's some work there that can be leveraged - or, better yet, perhaps that plugin can just be used as-is and Moonboots simplified a bit?

codeaholics commented 9 years ago

I've looked at this a bit more. Uglify has the ability to take an input sourcemap and transform it and produce an output map that maps back to the original source. I'll have a crack at implementing this over the next day or two and send a PR when it's ready.

codeaholics commented 9 years ago

I've got a working prototype of this, but I'm not convinced the mapping is correct, so I'll keep working on it for now.

Regards the question of what kind of source maps you want, I think you're right. Adding the map (especially if it's one with the sources embedded) to the end of the bundle negates the purpose of minification. I believe that the "right" end goal here is to have the map served separately by Moonboots in the same way as JS and CSS is. I'm happy to take this on. However, as you mentioned, this will require support from the Express middleware and HAPI plugin.

What do you think is the best way to approach this?

latentflip commented 9 years ago

Well, moonboots_hapi (I'm less sure about express but it'll be similar) consumes the following: https://github.com/HenrikJoreteg/moonboots/blob/master/index.js#L392-L426 methods from moonboots.

My understanding of how sourcemaps work is that the js file will have a comment pointing to the sourcemap (//# sourceMappingURL=/path/to/file.js.map) for example that must exist.

I think the best bet would be if moonboots also exported a jsSourceMapFileName() and jsSourceMapSource() functions (or similar) which moonboots_hapi can hook into.

@wraithgar any thoughts about that?

On 23 October 2014 16:42, Danny Yates notifications@github.com wrote:

I've got a working prototype of this, but I'm not convinced the mapping is correct, so I'll keep working on it for now.

Regards the question of what kind of source maps you want, I think you're right. Adding the map (especially if it's one with the sources embedded) to the end of the bundle negates the purpose of minification. I believe that the "right" end goal here is to have the map served separately by Moonboots in the same way as JS and CSS is. I'm happy to take this on. However, as you mentioned, this will require support from the Express middleware and HAPI plugin.

What do you think is the best way to approach this?

— Reply to this email directly or view it on GitHub https://github.com/HenrikJoreteg/moonboots/issues/57#issuecomment-60259268 .

Philip Roberts latentflip.com

codeaholics commented 9 years ago

Yes, the Express middleware works similarly.

I'm thinking at the moment, that serving the source map separately is the best way to go (regardless of whether minification is turned on or not) and that the source map should include the source files in it rather than referencing them back on the server. I've looked at the code, and I can handle all of these reasonably easily.

I'm more interested in how this might get released in a co-ordinated fashion with the Express and HAPI pieces, in terms of semver version numbers, etc.

Oh, and whether there is appetite for it at all! :-)

wraithgar commented 9 years ago

moonboots_hapi/express should send a X-SourceMap: /path/to/file.js.map header.

As for function names I'd go with

jsSource jsMap jsSourceName jsMapName

jsMap would include the source in it.

This would be a breaking change unless we kept jsFileName as a legacy alias for jsSourceName, but I'm ok w/o it cause it wouldn't cause a major version in the moonboots_hapi/express versions, only patch (since it's all under the hood at that level).

We would have to make sure that moonboots_hapi/express are peeking at sourceMaps boolean to see whether or not to include the header/route for maps.

codeaholics commented 9 years ago

I'd be inclined to leave it as a comment at the end of the JS bundle (//#sourceMapUrl=/path/to/file.min.js.map or whatever the syntax is). I was reading something the other day that suggested this has broader support than headers. It also means that HAPI/Express needn't know anything special about it - they just serve the JS exactly as they do now. They just need to have the ability in intercept the map file request and call back to Moonboots added to them.

wraithgar commented 9 years ago

Yes moonboots should do all the heavy lifting and hapi/express modules should just leverage new methods. Go ahead and PR your prototype if it's ready for a look-see and don't worry about the versioning we deal w/ that after merging.

codeaholics commented 9 years ago

OK, great. I think what I might do then is pop another PR through specifically to separate the source map out into a different request. That will allow the versioning issues, etc. to be hammered out separately from this issue.