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: enable multiple file types per entry #2213

Closed
wants to merge 0 commits into from

Conversation

trcarden
Copy link
Contributor

@trcarden trcarden commented Aug 5, 2019

Add support for multiple files per entry

Summary

It is possible to provide different types of files when using an array of values for entry to achieve separate bundles for CSS and JavaScript (and other) files in applications that are not using import for styles in JavaScript (pre Single Page Applications or different reasons).

For additional info: https://webpack.js.org/guides/entry-advanced/

Community Consideration

This is particularly useful if you are migrating from a per page
css/js organization as was common with rails <= 6

Note this has been a source of confusion and has been PR'd in the past as well. However no one pointed out this is a first class supported feature of webpack that is specifically built for static pages which rails does so well (see link above).

Here are just some of the PR/Issues from the past

PR changes

Seamlessly (without breaking support for the original single file per entry model) extends the entryPoint algo to extend entrypoints with multiple files when they are found.

So that you can get something like this

  'views/store/static/home': [
    '[redacted]/packs/views/store/static/home.js',
    '[redacted]/packs/views/store/static/home.scss'
  ]

and get a single pack with page specific js and css.

@trcarden
Copy link
Contributor Author

trcarden commented Aug 5, 2019

i will come back to check on this in a day or two. If at a high level people are ok with this extension I will put a little spit shine on there and add the tests.

@jakeNiemiec
Copy link
Member

jakeNiemiec commented Aug 5, 2019

This is particularly useful if you are migrating from a per page
css/js organization as was common with rails <= 6
...

'[redacted]/packs/views/store/static/home.js'

There have been a lot of users who incorrectly put all of their frontend assets into the /packs folder. Since there is a significant overhead to webpack entry files, this often leads to out-of-memory errors, frustration, and long build times.

As Gaurav points out in those tickets, the /packs folder is only for entry files that can be compiled to JS.

style-loader is also incompatible with your approach: webpack-dev-server won't be able to inject styles during development if you don't import styles in a JS file (HMR or otherwise).

Edit: This is not just a webpackER problem, I have had similar problems in vanilla webpack as well.

@trcarden
Copy link
Contributor Author

trcarden commented Aug 9, 2019

@jakeNiemiec thanks for the update.

I am working through the details with the rest of my team with regards to the style-loader issue you raised. However I wanted to acknowledge that everything you and @gauravtiwari have said both here and in other tickets make good sense.

The only thing I would ask you consider in addition to the great webpackER defaults/conventions, that we still allow people who want or need to use other features of webpack access to them. Without some kind of patch like mine you just can't do multi file entry points with webpackER (unless you monkeypatch or otherwise edit the code at runtime). I know there are workarounds where you can link your styles into your one JS entry point file but those don't work well for our use case. If we don't want this patch to go in as is perhaps we could open up some kind of a config or setting option to enable multi file entry points (opt-in) for the community. For those of us that don't need a JS render pipeline and just want sprinkles of JS (while keeping our packs to 1 per page with css) this multi file entry point was designed perfectly for that case.

I'll drop another update once we have finished our regression testing however right now this path (especially with chunking) provides amazing speed improvements over the sprockets + turbolinks approach.

@jakeNiemiec
Copy link
Member

Without some kind of patch like mine you just can't do multi file entry points with webpackER

Speaking as someone who rolls with a customized version of webpacker in production, I know where you are coming from.

Think of /packs as an abstraction of entry points where:

  entry: {
    home: ['./home.js', './home.scss'],
    account: ['./account.js', './account.scss']
  },

Turns into:

app/javascript:
  ├── packs: # Think of this as entry
  │   └── home.js
  │   └── home.scss
  │   └── account.js
  │   └── account.scss

Then in your home view:

<%= javascript_pack_tag 'home' %>
<%= stylesheet_pack_tag 'home' %>

I'm pretty sure that even webpack-dev-server works with this flow.

Could you tell me a bit about what you would like to change within the context of this flow? (e.g.: actual input/output vs desired input/output)

@trcarden
Copy link
Contributor Author

trcarden commented Aug 13, 2019

I originally tried something very similar. We opted to go with a js and css file per page for our migration to webpack. We then import in libraries and styles into the JS and SCSS as necessary from outside of the packs directory. Which creates something very similar to what we are used to from the old sprockets days. With chunking enabled we effectively get super lightweight, intelligently cached, extraordinary simple convention for per page assets (not to far from what sprockets originally gave us but with all of the webpack bells & whistles) Here is our directory structure:

app/webpack_assets: #renamed from javascripts to webpack_assets
  ├── packs: # Think of this as entry
  │   └── home: # Matches our rails view hierarchy
  │       └── index.scss # Matches the action name scheme from rails
  │       └── index.js

However when you compile a scss file you get some js file that overrides the js file you had intended. This is fixed however if you apply the patch I created as it explicitly tells webpack that you want both files (this is my working assumption based on the docs, I haven't confirmed that in the webpack(er) source yet)

It is likely related to:
webpack-contrib/mini-css-extract-plugin#12
https://stackoverflow.com/questions/48983748/how-to-prevent-webpack-to-create-js-file-for-css

If you have a way to get the same behavior (leveraging something like this https://webpack.js.org/guides/entry-advanced/) I would love to use it.

(unrelated to this specific issue, but still exciting, if you apply this strategy your compile times for any given page with or without the dev server drop dramatically with the --entry webpack flag as well (that is something we have as a patch within the webpacker gem but we understand that is something a bit more unique to us).

@jakeNiemiec
Copy link
Member

Matches the action name scheme from rails

Rails/Basecamp devs have a new framework that matches what is seems like you want to do: https://github.com/stimulusjs/stimulus

Copy link
Member

@jakeNiemiec jakeNiemiec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you create some tests?

@trcarden
Copy link
Contributor Author

trcarden commented Aug 16, 2019

@jakeNiemiec Of course I can add some tests, however does that mean you think there is a chance we will merge this PR/concept? Do you think we should do anything around config too or just leave it as is?

WRT Stimulus: We have looked at that for a while and it is a cool idea and maybe we could try that out on one of our greenfield projects. For now however I need to get webpack(er) working with my 100s of view specific js and css. woohoo. haha

@jakeNiemiec
Copy link
Member

does that mean you think there is a chance we will merge this PR/concept?

It's not that your idea is without merit, it just feels like an XY_problem that cuts against the grain inherent to webpack (packing a bunch of files into just a few), but, if it does not negatively impact webpacker at all, I don't see a reason why it couldn't be merged.

I need to get webpack(er) working with my 100s of view specific js and css

Ideally, you could have a single pack file set up to lazy/dynamically load any of the hundreds of js/css files you need for a single view. This way, you can chunk/cache/prefetch/tree-shake your files and skip the webpack entry overhead.

Read #1967 (comment) for a decent how-to on this. Otherwise, I feel like stimulus or even a basic xyz.js.erb in your view folders would work better for what you describe.

@trcarden
Copy link
Contributor Author

Awesome, Ok i'll add some tests in there and try to get this ready to ship.

WRT to the 100s of view specific js and css files. I think I may have miscommunicated. It's not that all of these files are included on the same view. Rather we have 100s of views with their own special combination of libs and specific data. We just want 1 scss and 1 js file per entry point and in those files we pull in whatever we need, then we let webpack do its chunking and tree shaking etc. I was just trying to convey we probably can't switch over to stimulus just yet on this large of a code base in one go (aka we still would love to have this feature).

Many thanks @jakeNiemiec ! I'll get to work on this and try to have it ship shape by EOW.

@jakeNiemiec
Copy link
Member

Many thanks @jakeNiemiec ! I'll get to work on this and try to have it ship shape by EOW.

No problem, tests are currently broken so there's plenty of time 😅

@trcarden
Copy link
Contributor Author

@jakeNiemiec Changed impl to ensure underlying representation would not change for people using the single file entry points. Also noticed the testing for this module is pretty sparse, I know Rails usually keeps testing more "pragmatic" so I added the happy path for the added functionality, happy to add a few corner cases but may need to create a few more files.

Let me know if you would like any edits (also I am not averse to you editing on my behalf if you prefer).

🍻Cheers 🍻

@jakeNiemiec jakeNiemiec self-requested a review August 26, 2019 20:38
@trcarden
Copy link
Contributor Author

@jakeNiemiec just checking in on this guy. Anything else that we need to do? I have a branch that we are using that leverages these changes but I would like to see us merge this or make updates as required so we can move on

@trcarden
Copy link
Contributor Author

trcarden commented Jan 6, 2020

@jakeNiemiec 2020 is here, I am just going through my list of open tickets. Any word on this?

@jakeNiemiec jakeNiemiec removed their request for review January 6, 2020 23:02
@jakeNiemiec jakeNiemiec dismissed their stale review January 6, 2020 23:03

addressed

@jakeNiemiec jakeNiemiec self-assigned this Jan 8, 2020
@trcarden
Copy link
Contributor Author

trcarden commented Mar 3, 2020

@jakeNiemiec sorry, where did we leave this again? I thought I saw a email come in from you about something regarding this. The base of this commit needs updating to the latest webpacker version. Is this fixed in some other way upstream now?

@trcarden
Copy link
Contributor Author

trcarden commented Mar 3, 2020

@jakeNiemiec sorry made a mistake, reopening under #2476 , also includes updates from 4.2.2 rebased under it.

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

Successfully merging this pull request may close these issues.

2 participants