Skip to content
This repository has been archived by the owner on Dec 1, 2020. It is now read-only.

WIP: Automatically insert node_modules as externals #22

Merged
merged 1 commit into from
May 14, 2019
Merged

WIP: Automatically insert node_modules as externals #22

merged 1 commit into from
May 14, 2019

Conversation

chopfitzroy
Copy link
Contributor

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
Copy link
Contributor Author

chopfitzroy commented Apr 2, 2019

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
Copy link
Contributor Author

@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
Copy link
Owner

kazupon commented Apr 23, 2019

Thank you for your PR!
Sorry my late reply 🙇
I will check your PR soon!

@msalahz
Copy link

msalahz commented Apr 23, 2019

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
Copy link
Owner

kazupon commented Apr 30, 2019

@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
Copy link
Contributor Author

@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
Copy link
Owner

kazupon commented May 1, 2019

@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
Copy link
Contributor Author

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
Copy link
Owner

kazupon commented May 3, 2019

@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 #1 (comment)

@chopfitzroy
Copy link
Contributor Author

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

@kazupon
Copy link
Owner

kazupon commented May 13, 2019

@CrashyBang
Hi
Sorry, late my reply. 🙇

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

@kazupon
Copy link
Owner

kazupon commented May 13, 2019

@CrashyBang
Also, if there is no fix at this current PR, please comment. 😉

@chopfitzroy
Copy link
Contributor Author

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!

@kazupon kazupon merged commit c519d43 into kazupon:master May 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants