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

WIP: feat: use multiEntry input #276

Conversation

chmelevskij
Copy link
Contributor

Another take on multientry input. Keeping in draft as it needs more tests

@pull-assistant
Copy link

pull-assistant bot commented Apr 29, 2020

Score: 0.99

Best reviewed: commit by commit


Optimal code review plan

     feat: use multiEntry input

     test: add tests for nested imports

Powered by Pull Assistant. Last update d2c09a2 ... 9701b53. Read the comment docs.

@codecov
Copy link

codecov bot commented Apr 29, 2020

Codecov Report

Merging #276 into master will increase coverage by 0.29%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #276      +/-   ##
==========================================
+ Coverage   92.47%   92.77%   +0.29%     
==========================================
  Files          10       10              
  Lines         319      332      +13     
  Branches      112      115       +3     
==========================================
+ Hits          295      308      +13     
  Misses         23       23              
  Partials        1        1              
Impacted Files Coverage Δ
src/index.js 99.09% <100.00%> (+0.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b17c15d...6e8a145. Read the comment docs.

@chmelevskij
Copy link
Contributor Author

chmelevskij commented Apr 29, 2020

@atompkins @BogdanDarius @daviddelusenet @MichaelAnd @emjaksa

this PR is based on the previous one with fixes for the empty imports. I passes the current test suite, but still needs more tests. Could you please verify it works for you and if possible provide some repro of your setups?

Related: #271 #264

@chmelevskij chmelevskij force-pushed the feat/support-multiple-entry-points branch from fd456ef to af4c6bc Compare April 29, 2020 07:45
@chmelevskij chmelevskij force-pushed the feat/support-multiple-entry-points branch from af4c6bc to d2c09a2 Compare April 29, 2020 07:47
@coxmi
Copy link
Contributor

coxmi commented Apr 29, 2020

I've just quickly swapped this out on the project i'm working on at the moment (so not a slimmed down case), but it's come through with an empty CSS file again:


/*# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJzb3VyY2VzIjpbXSwibmFtZXMiOltdLCJtYXBwaW5ncyI6IiIsImZpbGUiOiJzaXRlLmNzcyJ9 */

but strangely, with a corresponding site.css.map file:

{"version":3,"sources":[],"names":[],"mappings":"","file":"static/site.css"}

This setup probably isn't regular (custom static site gen plugin), so I'll try and get you a slimmed down reproduction later on.

Here's the css part of my conf at the moment:

const css = (emit = true, prod = false) => postcss({
    plugins: [
        emit && postcssImport(),
        emit && postcssPresetEnv({ stage: 0 }),
        emit && postcssCopy({
            dest: './dist/static',
            template: '[name].[hash].[ext]'
        }),
    ],
    extract: emit && 'static/site.css',
    minimize: prod,
    sourceMap: true,
    use: {
        less: { 
            // allows url asset paths to be relative
            rewriteUrls: 'all',
            // allows same aliases as js imports
            plugins: [ new LessAlias({ aliases: noAt() }) ]
        }
    }
})
const staticGen = {
    input: [
        'src/pages/index.js'
    ],
    output: {
        // output to site directory
        dir: 'dist',
        format: 'cjs',
        sourcemap: 'inline'
    },
    plugins: [
        …
        css(),
        …
    ]
}

export default [staticGen]

@coxmi
Copy link
Contributor

coxmi commented Apr 29, 2020

I've put a stripped-down build here: https://github.com/michaeland/postcss-test

to test, I cloned your repo, switched to the support-multiple-entry-points branch, ran npm run build and then yarn link’ed it into my test project.

On the face of it, it seems like it doesn't like any css imports that aren't at the top level. For example, if I include another js file from the main entry point:

in src/index.js:

import anotherSrc from './another.js'

in src/another.js:

import './test.less'

That results in an empty dist/site.css:


/*# sourceMappingURL=site.css.map */

but if I skip out the another.js import, and do it directly (in src/index.js):

import './test.less'

That results in the correct css file:

body {
  background: red;
}

/*# sourceMappingURL=site.css.map */

Hope that helps!

@chmelevskij
Copy link
Contributor Author

in src/index.js:

import anotherSrc from './another.js'

Had to add console.log(anotherSrc) because otherwise module gets tree-shaken. But still the same issue.

@ops1ops
Copy link

ops1ops commented May 12, 2020

Hi, how soon are you planning to release this feature?

@chmelevskij
Copy link
Contributor Author

Hi, how soon are you planning to release this feature?

Can't say for sure. There are some issues from rollup v2 which make it a bit more complicated to work for all the current cases.

@chmelevskij
Copy link
Contributor Author

Closing this as there are alternatives for this plugin. Found out that https://github.com/Anidetrix/rollup-plugin-styles does the 80% of what I needed so won't be continuing with this as there are a lot of thing which need to be updated for it to work correctly.

@ops1ops
Copy link

ops1ops commented Jun 10, 2020

and where is multy entry support in rollup-plugin-styles? I didnt find any example of it

@chmelevskij
Copy link
Contributor Author

It uses the fact that rollup has ability to understand input as object E.G.

 input: {
   entry1: 'path/to/entry1.js'
   entry2: 'path/to/entry2.js'
  // and so on
}

Any of the styles used in those entries will be genrated and put next to the entry. To set asset name you can use assetFilenames this also understands slashes in it and will create corresponding directories.

From personal usecase

const moduleOutput = {
    format: 'esm',
    dir: 'dist',
    entryFileNames: 'resources/vertical/[name]/LOCAL/index.mjs',
    chunkFileNames: 'vendor/[name].[hash].js',
    assetFileNames: 'resources/vertical/[name]/LOCAL/index[extname]',
    sourcemap: true,
};

hope this helps

@ops1ops
Copy link

ops1ops commented Jun 10, 2020

 input: {
   entry1: 'path/to/entry1.js'
   entry2: 'path/to/entry2.js'
  // and so on
}

is there any way to rewrite it with regexp? for example:

input: ['path/to/*.js]

i did not find, thats why have been using rollup-plugin-multi-input, but it does not create this object in the end and as the result rollup creates only 1 css file

@chmelevskij
Copy link
Contributor Author

I don't think it supports the regexes or minimatch patterns. At least I haven't tried and it's not mentioned in documentation.

In my case it's a mono-repo where every package is build separately and is corresponding to module in the app. Rollup config is just a plain javascript, so you can implement some sort of lookup for your modules and build the input dynamically if you know the structure of folders.

rollup-plugin-multi-input by definition allows you to input multiple files and output one output. Where is object in input will create separate bundles for every entry and will create separate chunks for re-used code.

It it definitely hard to tell why the output is single file without knowing your config for both inputs and outputs.

@ops1ops
Copy link

ops1ops commented Jun 10, 2020

Rollup config is just a plain javascript, so you can implement some sort of lookup for your modules and build the input dynamically if you know the structure of folders.

to my mind this should be done by library

It it definitely hard to tell why the output is single file without knowing your config for both inputs and outputs.

i have multiple files and every of them should be as output file and what you advised

 input: {
   entry1: 'path/to/entry1.js'
   entry2: 'path/to/entry2.js'
  // and so on
}

is not the best solution
i understand that i can implement it by myself and even dynamically read directory structure and create input objects depending on it, but as i said i think it should be already implemented in library and currently im looking for it.

@chmelevskij
Copy link
Contributor Author

Someone already had similar issue in rollup issues

I think it's possible to achieve what you want with @rollup/multi-entry and setting entryFileNames, assetFilenames and chunkFileNames.

Build tools shouldn't do everything, rollup provides excellent plugin system to prevent bloat in its code.

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

Successfully merging this pull request may close these issues.

3 participants