davestewart / vuex-pathify

Vue / Vuex plugin providing a unified path syntax to Vuex stores
https://davestewart.github.io/vuex-pathify
MIT License
1.37k stars 57 forks source link

WARNING Module not found: 'vue-class-component' #50

Closed ChrisExP closed 5 years ago

ChrisExP commented 5 years ago

I am not using Vue Class Components so when I run the build it works but it displays a warning message:

WARNING in ./node_modules/vuex-pathify/dist/vuex-pathify.esm.js
Module not found: Error: Can't resolve 'vue-class-component' in '.../node_modules/vuex-pathify/dist'
@ ./node_modules/vuex-pathify/dist/vuex-pathify.esm.js 1072:20-50

The line in question is:

try {
  createDecorator = require('vue-class-component').createDecorator;
} catch(e) {
  // Decorators are not available, so there is no need for decorators.
  createDecorator = function () { throw new Error("'vue-class-component' is required for decorators. Use 'npm -i vue-class-component'") };
}

Is there any way to use pathify without Vue Class Components and silence the warning message?

davestewart commented 5 years ago

Ah, that sucks.

This is a new addition to 1.2. I didn't actually write the code, so let me take a look but also shout out to @ozum who did...

But yes, you're right, you don't need this lib to use it.

davestewart commented 5 years ago

Hmmm. (I deleted the comment as I realised it wouldn't make any difference)

So I think @ozum's intention was for the try/catch to swallow that error. I don't know enough about the technique to solve it right now, but you're saying it DOES build?

Look like this is a standard way to do things:

I'll wait for @ozum to comment.

ozum commented 5 years ago

@davestewart, @nal-chris

As of 1.2 vuex-pathify supports decorators. To make this happen, vue-class-component is used.

Class based vue projects, which may use decorators already have vue-class-component in their bundle. Making bundle smaller for those not using decorators (like in case of @nal-chris), I didn't include vue-class-component in package.json. I used try-catch block to catch missing requirement and let the code work as intended without vue-class-component.

During development, I thought 3 possible ways to implement decorators:

  1. Adding vue-class-component to package.json without try-catch and have bigger bundle.
  2. Not adding vue-class-component to package.json and handling it with a try-catch to have smaller bundle.
  3. Deleting decorators support from vuex-pathify and creating another package for decorators such as vuex-pathify-decorators. In that case @davestewart have to maintain 2 modules or vuex-pathify-decorators have to rely another maintainer and have risk of out of sync.

I assumed number 2 is best of both (smaller bundle - single module). However I'm not an expert on front end bundling and open better alternatives and suggestions.

ChrisExP commented 5 years ago

I agree a smaller build should be prioritised, although the vue-class-component package is only about 300 lines unminified so it wouldn't have too much impact.

The best alternative I can come up with is to have the user pass the createDecorator function in the config, then the package wouldn't need to require it.

import pathify from 'vuex-pathify'
import { createDecorator } from 'vue-class-component'
export default pathify

// options
pathify.options.mapping = 'simple'
pathify.options.deep = false
pathify.options.createDecorator = createDecorator

This is less ideal as it "punishes" people using vue-class-component.

The other option I thought of was to import just the createDecorator function from the vue-class-component source code, then you could add it to package.json but you'd only be adding about 40 lines from the util.js file:

createDecorator = require('vue-class-component/src/util').createDecorator

However I don't know how feasible that is and think that would also have issues. You wouldn't be able to throw the error if the user hasn't imported the rest of the package, plus you would need to include some of the dev dependencies that vue-class-component uses in order to make it work.

davestewart commented 5 years ago

Passing createDecorator could certainly be an option:

https://davestewart.github.io/vuex-pathify/#/setup/config

This is less ideal as it "punishes" people using vue-class-component.

It's only one line of setup!

As for @ozum's option 3; that is another avenue, but the options option seems good.

@ozum - did you work out why the build error occurs, even with try/catch ?

ozum commented 5 years ago

@davestewart I didn't use rollup before, but according to https://github.com/rollup/rollup/issues/1385, try-catch conditional require should work as of rollup 0.43. I didn't use rollup before, could you please submit a question to rollup?

@nal-chris and @davestewart pathify.options.createDecorator = createDecorator is good idea, but I think regular vue developers (non vue module/add-on developers) may not be aware of createDecorator. It could be annoyance for them.

As @nal-chris pointed out, option 2 seems sub-optimal. If rollup community cannot provide a solution, I think option 3 is the way to go. (it is also suggested on https://github.com/rollup/rollup/issues/1385, before rollup 0.43)

ChrisExP commented 5 years ago

I don't think the issue is with rollup. Rollup builds the package but then in my code I'm using webpack to import the package into my project.

The code in webpack looks like this:

const errorOrWarningAndCallback = err => {
    if (isOptional()) {
        return warningAndCallback(err);
    } else {
        return errorAndCallback(err);
    }
};

Where isOptional() detects if the dependency is in a try/catch block (I think). So it builds but it still throws a warning.

ozum commented 5 years ago

@nal-chris with my limited knowledge about bundling, according to generated code, it is impossible to overcome warning in webpack then, at least with your configuration.

Could you please check if there are config or plugin for use optional requirements for try-catch? try-catch is widely used pattern for optional requirements, and there should be a solution.

davestewart commented 5 years ago

I've been thinking a bit more about this, and my gut is telling me to go with a separate package, however what if I decide to switch to TypeScript in future?

With a separate package, it would simply be a case of:

import { Get, Sync, Call } from 'vuex-pathify-decorators`

If it was a separate package, @ozum, would you want to be the author and take responsibility for its releases on NPM, or shall I release it and make you a contributor?

I'll find the time this week to try both approaches then we can tick this one off.

ozum commented 5 years ago

I've been thinking a bit more about this, and my gut is telling me to go with a separate package, however what if I decide to switch to TypeScript in future?

@davestewart if you decide for separate packages, you can unify them again in the future with a major version release when you decide to switch to typescript.

With a separate vuex-pathify-decorators package you may import and reexport every public attributes from vuex-pathify similar to vue-property-decorator does for vue-class-component. So a developer should include only one of them, either vuex-pathify-decorators or vuex-pathify.

If it was a separate package, @ozum, would you want to be the author and take responsibility for its releases on NPM, or shall I release it and make you a contributor?

Even for separate packages I think you should author both packages, because it's a small core functionality and both package must be in sync always.

I would...

I think it's best to test bundle size of vuex-pathify with and without vue-class-component when you have time. If it differs small enough (@nal-chris reported it is 300 lines of code), add vue-class-component to dependencies. Otherwise make them separate.

davestewart commented 5 years ago

Cool. Thanks for your input!

vesper8 commented 5 years ago

locking this package to 1.2.0 gets rid of the warning. Warning was only introduced as of 1.2.1

hope this gets sorted soon : )

davestewart commented 5 years ago

I've had a whole load of deadlines for the last few weeks - I'll try and get to it this week, as I can see it's quite annoying.

ozum commented 5 years ago

Hi @davestewart.

I see that, this issue annoys people, and as @nal-chris pointed out

the vue-class-component package is only about 300 lines unminified so it wouldn't have too much impact.

Also vue-class-component, has no other dependencies, so impact is minimal. I created a new PR which:

I think this PR should solve the problem.

@nal-chris, could you check please?

davestewart commented 5 years ago

I'm happy to publish this as a patch release then we can always look again at this with a breaking version number if we need to pull it out or anything.

davestewart commented 5 years ago

OK, 1.2.2 is published on NPM.

Let me know how you go.

ozum commented 5 years ago

I briefly tested 1.2.2 from NPM, and it seems well without warning.

@nal-chris, @vesper8 could you please confirm?

chiboreache commented 5 years ago

Works like a charm!

davestewart commented 5 years ago

Yay!

Sorry I don't spend that much time on this lib these days, and thanks @ozum for stepping up.

ChrisExP commented 5 years ago

It works for me. Thank you very much!