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

Feature Request: Given a multi-entry-point configuration, output multiple chunks with known names as generated by Rollup #13

Open
reubenrybnik opened this issue Apr 24, 2021 · 4 comments

Comments

@reubenrybnik
Copy link

  • @rollup/stream Version: 2.0.0

Feature Use Case

I'm using Rollup to bundle node dependencies specifically (not an entire app), and to do that efficiently it is useful to define multiple named entry points in a single config. With this approach, dependencies of dependencies which are sometimes shared between dependencies and thus can benefit from having multiple entry points that are evaluated together are also efficiently output as separate chunks as necessary via code splitting. The current examples, however, don't appear to support this output well since with them it seems like I can only have one file output and I need to know what it's called. To be able to do this efficiently, I believe that I need to have a mode for this plugin in which it wraps the files in Vinyl streams itself to provide multiple file outputs and preserve the names of those files.

The reason I'm not using gulp-rollup for this is that it seems to imply that I'd need to load every file that Rollup might need in gulp.src, which seems inefficient, particularly because I'd ideally also want to avoid pulling in dev dependencies. I could maybe avoid providing any source files at all using this option, but that seems to be discouraged 🤣

The rollup.config.js that I'm currently using for this is below:

var replace = require('@rollup/plugin-replace');
var nodeResolve = require('@rollup/plugin-node-resolve');
var commonjs = require('@rollup/plugin-commonjs');
var babel = require('@rollup/plugin-babel');

var config = {
    // the entry points for each item to bundle
    input: {
        '@date-io/moment': './node_modules/@date-io/moment/build/index.js'
        '@material-ui/core': './node_modules/@material-ui/core/esm/index.js',
        '@material-ui/lab/Autocomplete': './node_modules/@material-ui/lab/esm/Autocomplete/index.js',
        '@material-ui/pickers': './node_modules/@material-ui/pickers/index.js',
        'react': './node_modules/react/index.js',
        'react/jsx-runtime': './node_modules/react/jsx-runtime.js',
        'react/jsx-dev-runtime': './node_modules/react/jsx-dev-runtime.js',
        'react-dom': './node_modules/react-dom/index.js',
        'react-infinite-scroll-hook': './node_modules/react-infinite-scroll-hook/dist/index.js',
        'react-merge-refs': './node_modules/react-merge-refs/dist/index.js'
        'redux': './node_modules/redux/es/redux.js',
        'react-redux': './node_modules/react-redux/es/index.js',
        'redux-thunk': './node_modules/redux-thunk/es/index.js',
    },

    plugins: [
        // replace node environment variables with appropriate values
        replace({
            preventAssignment: true,
            values: {
                'process.env.NODE_ENV': JSON.stringify(process.env.NODE_ENV)
            }
        }),

        // resolve named dependencies using node resolution rules
        nodeResolve.nodeResolve({
            browser: true
        }),

        // convert CommonJS modules to ES6 so Rollup will understand them
        commonjs(),

        // use babel to read and transpile modules, which may contain stuff like JSX
        babel.babel({
            babelHelpers: 'bundled',
            extensions: [
                '.js',
                '.jsx'
            ]
        })
    ],

    output: {
        // prefix dependency chunk bundles with __chunk_ to prevent conflicts
        chunkFileNames: '__chunk_[name]/js/index.js',

        // output all entry point bundles as AMD modules to js/index.js under the module's path
        entryFileNames: '[name]/js/index.js',

        // output modules in AMD format - our codebase is old and we'd like to eventually move
        // away from this but we can't yet
        format: 'amd',

        // use ES interop
        interop: 'auto'
    }
};

// reference entry point dependencies by name rather than relative path
config.external = Object.keys(config.input);

module.exports = config;

Feature Proposal

It would be nice if this plugin offered an option to provide multiple chunks from Rollup as separate outputs in a format that's easy to consume since it seems like such output isn't available in the current version. I'm interested in potentially contributing a change that accomplishes this goal back to the main repository if this is a feature that maintainers are interested in adding or possibly documentation that shows how to use this plugin to accomplish such a goal if this feature already exists and I missed it.

I've put together a proposed implementation at master...reubenrybnik:output-vinyl-stream that is comprised of an optional flag which when specified causes this plugin's output to be changed to a Vinyl object stream to which the separate named files provided by Rollup are written. I'm currently using this implementation in a private commercial repository. This works for my use case but may not be the best implementation of this feature from maintainers' perspective, and I'm happy to discuss potential alternatives in that case.

If this approach is accepted, I believe my current proposed implementation of adding an option to switch to this output mode and making the Vinyl dependencies optional would make this a non-breaking change. However, I'm interested in knowing maintainers' views on whether it might be better to move away from the current pattern of using vinyl-source-stream to convert the output to a single Vinyl item. In particular, I'm not sure whether there are any use cases in which this output is not immediately converted to Vinyl or whether maintainers are interested in supporting such use cases. I'm also interested in views on whether it would be better to switch to this mode using an option as in my current implementation or whether it might be better to provide a separate function property off of the main export instead (i.e. something like webpackStream.vinyl(config) instead of webpackStream(config, { outputVinyl: true })). I'd also welcome any other feedback you'd like to provide on this approach.

Thanks in advance for your consideration of this request!

@waynevanson
Copy link

I believe you should add this as a PR, even as a draft. That way it can be reviewed.

I believe through2 is no longer required, as it can be replaced with Node's simplified stream API constructor.

new Transform({
  objectMode: true,
  transform() { /*... */ }
})

I feel in this case, the PR should remove Vinyl and stream the chunks (RollupOutput["output"]) instead. This way a wrapper can be made to implement transforming each chunk to a vinyl instance. To activate it, set a flag in the options to true (options.objectMode).

Related links:

I've been sculpting a gulp plugin here: https://github.com/waynevanson/rollup-stream-gulp

I made an issue regarding a gulp plugin here https://github.com/rollup/rollup/issues/4136

@reubenrybnik
Copy link
Author

reubenrybnik commented Jun 13, 2021

Thanks for the feedback @waynevanson, and it's nice to see your interest in helping Rollup to work better in a Gulp pipeline in rollup/rollup#4137! I like the approach you've suggested to avoid needing to add optional dependencies to this plugin by using native object streams and converting to Vinyl further down the stream similar to how vinyl-source-stream currently works; I'm not familiar with optional dependencies and how best to convey the circumstances under which they should be installed so not needing to include them will be nice.

Providing the ability to leave comments on specific lines probably would have been nice even for code that I knew was nowhere near the finish line, and I'll keep that in mind in the future. I'll probably start a new branch for these changes, though, since I don't think any of what I've currently written will survive aside from maybe the options modifications.

@reubenrybnik
Copy link
Author

I'm also happy to step back if you'd prefer to make this change yourself as part of your larger project @waynevanson; the modified version that I provided a diff for works for my needs, so I was just suggesting this in the interest of possibly contributing back to the community if such a contribution is desirable.

@shellscape
Copy link
Contributor

This isn't a very frequently maintained package. We'd be happy to review a PR from the community to resolve the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants