kazupon / vue-cli-plugin-p11n

:electric_plug: Vue CLI 3 plugin to pluginize your Vue project
MIT License
113 stars 13 forks source link

WIP: Automatically insert node_modules as externals #22

Closed chopfitzroy closed 5 years ago

chopfitzroy commented 5 years ago

As per #21 I am submitting a PR to allow the use of external libraries (such as axios) without having to include them in the final bundle.

The code I have submitted does work, however there are some caveats that I think need to be addressed before we merge.

1) If a user installs a package via npm without the --save flag it will not be included in the projects dependencies and as such will not be made automatically made external. 2) Currently to make it automatic or just work we make all dependencies external, an API should be made available if you do not wish to make a dependency external. 3) Currently you can not add additional externals (outside of what is listed in the project dependencies) I think an API should be made available to do this, and it can be merged with the automatically generated externals. 4) esm and umd modules require a global to be set when using externals (see here) Rollup does it's best to guess what the global name should be, but an API should be provided where a user can manually specify these if needed (there are some examples here of when guessing does not work).

If you are interested in the difference between global and external see here.

Based on these requirements I suggest we add some sort of config file (.p11nrc for example) with the following options (assuming JSON style format):

"externalBlacklist": ["is-object"],
"externalAdditional": ["my-custom-module"],
"globalManual": {
    "react": "React",
    "react-router": "ReactRouter"
}

The way I see this working or the flow would be as follows:

1) Construct list of externals from dependencies. 2) Omit the externalBlacklist from constructed list. 3) Add any non dependencies externals to the constructed list. 4) Insert externals and globals into generated config.

A rough example is below (using lodash):

function generateConfig (options, moduleName, version, langInfo) {
    const config = require(path.resolve(process.cwd(), '.p11nrc'))
    const { dependencies } = require(path.resolve(process.cwd(), 'package.json'))
    const plugins = buildinPlugins(version, options.env, langInfo)
    const externalsOmitted = _.omit(dependencies, config.externalBlacklist)
    const external = [...Object.keys(externalsOmitted), ...config.externalAdditional]
    const globals = config.globalManual

    return { 
        input: options.entry,
        output: {
            file: options.dest,
            name: moduleName,
            format: options.format,
            banner: options.banner
            // TODO: sourcemap: 'inline'
        },
        plugins,
        external,
        globals
    }
}
chopfitzroy commented 5 years ago

NOTE: I had a really hard time testing this locally, I couldn't just remove the package (in package.json and package.lock) and npm link I had to leave the package installed and the rm -rf it from node_modules and then npm link my local copy...

Not sure if this is to do with how the vue-cli-service reads things from package.json but if you have any advice on how to do this would be glad to hear it :)

chopfitzroy commented 5 years ago

@kazupon After thinking about this further the above mentioned config file is the only way I can think of the solves this problem, I am happy to tool this out but I would like confirmation that this is the right direction for the project before I write any additional code.

If it is not please let me know what you think and I can start working on an alternative solution.

kazupon commented 5 years ago

Thank you for your PR! Sorry my late reply ๐Ÿ™‡ I will check your PR soon!

msalahz commented 5 years ago

I will be appreciated if I can specify that a dependency x should be bundled during plugin build or not, because now they are all excluded by default.

kazupon commented 5 years ago

@CrashyBang I think that the module dependencies problem should be solved not by vue-cli-plugin-p11n but by the user setting custom rollup.config.js.

To add APIs and configuration file to vue-cli-plugin-p11n, JavaScript module dependencies becomes more complicated.

In about #1, I wanted to support importing customized rollup.config.js with user. Specifically, vue-cli-plugin-p11n will merge the imported rollup.config.js settings into the internal settings. By supporting this feature, vue-cli-plugin-p11n simplifies the implementation. users don't need to learn new APIs and settings.

chopfitzroy commented 5 years ago

@kazupon sounds good to me, so with that in mind do we still want to use what I have to automatically populate the externals via the package.json if no rollup.config.js is available?

kazupon commented 5 years ago

@CrashyBang In about automatically node_modules as externals, I agree your PR!

If the user don't expect the resolution behavior of module dependencies, I think that want to be able to fully control it with rollup.config.js If that configuration exists, I think that want to be disabled the behavior of automatically externalizing.

What do you think about it?

As I noticed now, Vue CLI supports vue.config.js, so if you set it as a plug-in option as shown below, it seems that extra configuration files will not increase.

chopfitzroy commented 5 years ago

Hey @kazupon,

Sorry for the delay in my response, yes I totally agree if a rollup.config.js is present it should be ignored.

In regards to setting a vue.config.js option could you please explain what the option would do?

Cheers!

kazupon commented 5 years ago

@CrashyBang

In regards to setting a vue.config.js option could you please explain what the option would do?

I've updated #1 See the https://github.com/kazupon/vue-cli-plugin-p11n/issues/1#issue-405082451

chopfitzroy commented 5 years ago

Hey @kazupon I like that! Should wait for that functionality to exist before we merge this in?

kazupon commented 5 years ago

@CrashyBang Hi Sorry, late my reply. ๐Ÿ™‡

At first, let's merge this PR! After merging, we will try to support #1.

kazupon commented 5 years ago

@CrashyBang Also, if there is no fix at this current PR, please comment. ๐Ÿ˜‰

chopfitzroy commented 5 years ago

Hey @kazupon it all works so I am happy to merge it in, before we do though, I was wondering if this is the best way to reference the package.json:

const { dependencies } = require(path.resolve(process.cwd(), 'package.json'))

Otherwise I am happy to merge!