-
-
Notifications
You must be signed in to change notification settings - Fork 6
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
Request build & release with new unregister feature. #3
Comments
I've been investigating further with a fork. The built files using Non-prod build unit test error:
Prod build e2e test error: I reproduced this on a blank vue-cli/webpack project. Used the Vuec plugin as well as another plugin. Vuec is the only one that had a problem. I wonder if there's an incompatibility between the dist files emitted by Vuec and what is accepted by certain environments in the webpack template. Edit: Indeed, removing the So, the problem currently is that Vuec's prod dist files do not work with the vue-cli/webpack template's e2e or production build pipelines, due to the UglifyJsPlugin. I'll try to determine the root cause. |
Thanks for taking the time to investigate what's causing this problem! As for the unregister, I'll push out a new release with these features right away so you don't have to point to master for them 😄 |
Definitely, I can provide a live repo later for you to look at once I'm finished with meetings :P BTW, I replaced the UglifyJS plugin with a Babili plugin and saw the same problem. Went through various options and determined that the If UglifyJS mangles by default in Thanks for your attention! |
Here, this should work: https://github.com/daekano/vuec-proof Let me know if my instructions are unclear. |
I figured out the problem, the Vuec container relies on the parameter names to resolve the dependencies from the container, however when name mangling is enabled, the parameter names will be transformed. For example, in the example you provided the code looks like this after mangling: , function(t, e, i) {
"use strict";
Object.defineProperty(e, "__esModule", {
value: !0
}),
e.default = {
name: "hello",
data: function() {
return {
greeting: "false"
}
},
created: function(t) { // notice that the parameter was renamed to "t"
this.Service = t
},
beforeMount: function() {
this.greeting = this.Service.fetch()
}
}
} This will hold up for UglifyJS, Babili and pretty much every other minifier out there unless you'd turn off name mangling. I'm going to investigate if there's any way to selectively turn off name mangling and in the worst case I'll have to add a warning to the readme. In any case the container should probably throw an error in debug mode or print a warning or something like that, I'd be happy to hear your opinion on this Thanks for taking the time to report this! EDIT: for UglifyJS I found this and for Babili I found this which should most likely work but I have not tested this |
So I tested the solution I suggested above myself and added this to the webpack.prod.conf.js: new webpack.optimize.UglifyJsPlugin({
compress: {
warnings: false
},
sourceMap: true,
mangle: {
except: ['Service'], // blacklist your service from being mangled
}
}), which results in the following compiled code: , function(t, e, i) {
"use strict";
Object.defineProperty(e, "__esModule", {
value: !0
}),
e.default = {
name: "hello",
data: function() {
return {
greeting: "false"
}
},
created: function(Service) { // notice the parameter name was not mangled
this.Service = Service
},
beforeMount: function() {
this.greeting = this.Service.fetch()
}
}
} So you have to either disable name mangling (which in return results in a bit higher output size) or you'll have to blacklist your services in the name mangling configuration |
Ah, that makes total sense. So it's a downstream configuration problem, though Vuec can be more helpful to developers in the future. I will most likely be disabling name mangling. I'm not sure how I feel about application domain-related whitelists in build logic. Do you think it's possible to emit an error during debug mode? Nothing is being mangled and we don't have access to the production pipeline configuration settings. I don't think emitting a warning during production mode is helpful, since we'd probably be making a best guess as to whether something is being mangled or not. Just a static reminder during debug mode not to mangle names? Seems a little noisy to me. I'll think about the developer experience a little further. |
I think we'll go with a warning in the documentation and throwing an error in debug mode if the container fails to resolve a dependency (which is probably what I should have gone with in the first place rather than returning null) |
Not sure how many authors will be uglifying (let alone mangling) output in Vue's debug mode, but it could help someone someday. |
Probably not too many but in production it's the default setting so we should make sure they at least are aware of this caveat. I've been researching into using regexes for excluding from mangling so that you could for example exclude names starting with a "$" (like Angular) |
Cool. I'm more than willing to implement a fix that we agree on if you haven't already. In fact, it would be kind of nice to create my first OSS pull request :) |
Documentation has been updated to reflect these problems and an alternative way of registering changes has been implemented for people who don't want to deal with mangling. I'm closing this, but if you run into anymore problems or have other suggestions feel free to reopen this. |
Hello!
First off let me say I love the simplicity of this container plugin, and it works great.
The first problem I had was with the unregister method not being available. I had to point to the master branch for this.
Then, I had a problem with my webpack builds -- the container wasn't injecting registered dependencies during e2e/prod modes (vue-cli webpack template). They were all undefined within the lifecycle hooks. Pointing to src/index.js fixes the condition for e2e tests and prod mode, but then unit tests break.
I'm wondering if we can work together on building and publishing an npm release with these new features. If you have other items in scope for this release, I don't mind contributing to accelerate.
Thanks!
The text was updated successfully, but these errors were encountered: