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

Request build & release with new unregister feature. #3

Closed
defnorep opened this issue Apr 26, 2017 · 12 comments
Closed

Request build & release with new unregister feature. #3

defnorep opened this issue Apr 26, 2017 · 12 comments
Labels

Comments

@defnorep
Copy link

defnorep commented Apr 26, 2017

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!

@defnorep
Copy link
Author

defnorep commented Apr 26, 2017

I've been investigating further with a fork. The built files using $ webpack -p work fine for my unit tests, but fail for my e2e tests. The built files using $ webpack work fine for my e2e tests but not my unit tests.

Non-prod build unit test error:

PhantomJS 2.1.1 (Mac OS X 0.0.0) ERROR
  SyntaxError: Use of reserved word 'class'
  at webpack:///~/vue-container/dist/vuec.js:84:0 <- index.js:10932

Prod build e2e test error:
Injected dependencies are undefined.

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 webpack.optimize.UglifyJsPlugin instance from webpack.prod.conf.js allows both test environments to pass when using Vuec's dist files.

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.

@dealloc
Copy link
Owner

dealloc commented Apr 26, 2017

Thanks for taking the time to investigate what's causing this problem!
Do you happen to have a repository which demonstrates this issue so that I can play around with it a bit myself?

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 😄

@defnorep
Copy link
Author

defnorep commented Apr 26, 2017

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 mangle option breaks prod mode builds, both in Babili as well as UglifyJS.

If UglifyJS mangles by default in webpack -p then maybe we should consider disabling it and deferring the decision to application authors. (Or expect application authors to not mangle? Hmm.)

Thanks for your attention!

@defnorep
Copy link
Author

defnorep commented Apr 26, 2017

Here, this should work:

https://github.com/daekano/vuec-proof

Let me know if my instructions are unclear.

@dealloc
Copy link
Owner

dealloc commented Apr 27, 2017

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

@dealloc dealloc added the bug label Apr 27, 2017
@dealloc
Copy link
Owner

dealloc commented Apr 27, 2017

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

@defnorep
Copy link
Author

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.

@dealloc
Copy link
Owner

dealloc commented Apr 27, 2017

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)

@defnorep
Copy link
Author

Not sure how many authors will be uglifying (let alone mangling) output in Vue's debug mode, but it could help someone someday.

@dealloc
Copy link
Owner

dealloc commented Apr 27, 2017

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)

@defnorep
Copy link
Author

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 :)

@dealloc
Copy link
Owner

dealloc commented May 5, 2017

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.

@dealloc dealloc closed this as completed May 5, 2017
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

2 participants