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

Minified Dep Bundles are Broken #947

Closed
Sinjhin opened this issue Feb 13, 2018 · 9 comments
Closed

Minified Dep Bundles are Broken #947

Sinjhin opened this issue Feb 13, 2018 · 9 comments
Labels

Comments

@Sinjhin
Copy link

Sinjhin commented Feb 13, 2018

When building the CanJS site if I use "deps-bundle": "steal-tools bundle --deps" in my scripts and try to load a demo with that deps-bundle like:

<script id="demo-source" main="@empty" src="../../node_modules/steal/steal.js" deps-bundle>

It will break on load with the error:

steal.js:7390 Error: Error loading "dev-bundle" at http://localhost:8080/dev-bundle.js
TypeError: define is not a function
	    at Object.eval (http://localhost:8080/dev-bundle.js:4:125077)
	    at eval (http://localhost:8080/dev-bundle.js:5:4)
	    at eval (http://localhost:8080/dev-bundle.js:5:98)
	Evaluating http://localhost:8080/dev-bundle.js

If I add the --no-minify option "deps-bundle": "steal-tools bundle --deps --no-minify" it works fine but the deps-bundle is not minified.

@Sinjhin Sinjhin self-assigned this Feb 13, 2018
@matthewp
Copy link
Member

Can you recreate with an example app?

@m-mujica
Copy link
Contributor

This is a known issue, and it's the reason why we made minify false the default #905

To fix it, this regex needs to ignore any glob patterns introduced by this node https://github.com/stealjs/steal-tools/blob/master/lib/bundle/add_npm_packages_node.js

@Sinjhin
Copy link
Author

Sinjhin commented Feb 13, 2018

@m-mujica It looks like the minify false by default is not working in v1.11.3 unless I specifically add the --no-minify tag to the script.

@Sinjhin Sinjhin removed their assignment Feb 20, 2018
@matthewp
Copy link
Member

matthewp commented Mar 7, 2018

@m-mujica is there another existing issue for this?

@matthewp matthewp added the bug label Mar 7, 2018
@m-mujica
Copy link
Contributor

m-mujica commented Mar 7, 2018

@matthewp nope, I didn't check if there is indeed a bug with the default minify setting, there is a test here

describe("dev bundle build", function() {
this.timeout(5000);
var baseOptions = {
quiet: true
};
it("should not be minified by default", function() {
var config = {
main: "bundle",
config: path.join(__dirname, "bundle", "stealconfig.js"),
};
var bundlePath = path.join(__dirname, "bundle", "dev-bundle.js");
return devBundleBuild(config, baseOptions)
.then(function() {
return readFile(bundlePath);
})
.then(function(contents) {
// comments are removed during minification
var rx = new RegExp(escapeRegExp("/*[system-bundles-config]*/"));
var rx2 = new RegExp(escapeRegExp("/*stealconfig.js*/"));
assert(rx.test(contents), "bundle should not be minified");
assert(rx2.test(contents), "bundle should not be minified");
})
.then(function() {
return rmdir(bundlePath);
});
});

@kylegifford
Copy link

kylegifford commented May 25, 2018

I'd like to bump this issue. We have several clients using this, and it would be nice to have the additional speed improvements when developing. Here is an example from Lowe's, where minification reduces the size by over 35%:

screenshot 2018-05-25 16 51 09
screenshot 2018-05-25 16 51 25

@justinbmeyer
Copy link
Contributor

@kylegifford is it gzipped? gzipping often improves payload size even more than minification. Also, it looks like load is only effected about 60ms. While that's something, from the slack convo, it looks like you are looking for more substantive improvements?

@justinbmeyer
Copy link
Contributor

justinbmeyer commented May 26, 2018

instead of using a regexp ... I wonder if can-parse could perform well enough here, and be accurate enough. We don't need to support JS's full syntax, only it seems strings and comments?

@m-mujica
Copy link
Contributor

Just added a branch with a breaking test, might try to fix it this weekend... anyway, if anyone else picks it there you have the test.

#1003

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants