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

Follow app/javascript/application.js convention from Rails 7 #3156

Merged
merged 7 commits into from
Sep 4, 2021

Conversation

dhh
Copy link
Member

@dhh dhh commented Sep 3, 2021

To ensure compatibility across the different JavaScript bundler options supported by Rails 7 (like esbuild-rails), we need to use the same entry point by default. That entry point is going to be app/javascript/application.js. You can of course continue to change that in your webpacker.yml config, but by sharing the same default, we can make other generators work out of the box.

So it remains compatible with other bundler setups, like esbuild-rails.
We can add them via Rails 7 installers
@mcmire
Copy link

mcmire commented Sep 3, 2021

What's the reasoning behind this change? I feel like app/packs (or something like it) is a good name because it communicates to developers that it's not just JavaScript that can be bundled, it's also CSS, images, SVGs — you name it. I realize what can be bundled is wholly dependent on the bundler you use, but even esbuild supports bundling CSS. So couldn't this possibly be confusing? Or is the idea that most developers will want to continue to use app/assets for CSS, images, SVGs, etc. and the idea of using a JavaScript bundler for this is already confusing enough?

@dhh
Copy link
Member Author

dhh commented Sep 3, 2021

app/javascript is what Webpacker has used in its entire existence of release. The new importmap approach that'll be the default in Rails 7 uses the same, then app/assets for anything else. This default ensures that all the default integrations can work off the same entry point, and for those who want something else, the webpacker.yml file let's them change it.

@crawler
Copy link
Contributor

crawler commented Sep 3, 2021

So the "my_component.js" will be processed as the entry point. What if you have files that you don't want to be entry points, at least because of compile performance?

You will need to set up something like app/javascript_lib ?

@dhh
Copy link
Member Author

dhh commented Sep 3, 2021

@crawler Entrypoints are limited to the top level, so app/javascript by default. app/javascript/components (or whatever) is where you'll put anything not a entry point.

@crawler
Copy link
Contributor

crawler commented Sep 3, 2021

@dhh I see, not sure if this is suitable for everyone. Because at least we use directory structured entry points, as well as some custom libs. But anyway, this behavior can be changed with configuration/tweaking.
BTW i fixed github actions workflow in #3150

@dhh
Copy link
Member Author

dhh commented Sep 3, 2021

Yeah I don’t think we will find something that works for everyone in all cases. But Webpacker is quite flexible with the setup it has now. Easy to change!

@dhh dhh merged commit 5de0fbc into master Sep 4, 2021
@dhh dhh deleted the use-conventional-source-path branch September 4, 2021 10:07
@TylerRick
Copy link
Contributor

TylerRick commented Sep 11, 2021

FYI, when I tried this, I noticed that webpack ended up searching for files recursively under source_entry_path (app/javascript), so it ended up with >100 entrypoints listed under "entry":

  "entry": {
    "application": "app/javascript/application.js",
    "common/array/chunk": "app/javascript/common/array/chunk.ts",
    "common/http_interceptors/add_csrf_token": "app/javascript/common/http_interceptors/add_csrf_token.ts",
    "common/invertMap": "app/javascript/common/invertMap.tsx",
    "common/utils": "app/javascript/common/utils.ts",},

Unless there's some way to make it only look at the top level of app/javascript without recursing, or unless I'm just doing something wrong, then this may not be such a good idea after all...

As a workaround, I ended up changing the config back to a "dead-end" (no subdirs for it to recurse) directory:

  source_entry_path: entrypoints

@dhh
Copy link
Member Author

dhh commented Sep 11, 2021

Yes there’s a change to the JS glob that goes in the @rails/webpacker module only to look at the top level. Not nested.

@TylerRick
Copy link
Contributor

Thanks, that did the trick.

(yarn add https://github.com/rails/webpacker.git instead of locked to rc.5)

brandoncordell added a commit to brandoncordell/webpacker that referenced this pull request Oct 1, 2021
This should add back the ability to namespace packs which was removed in rails#3156 at the cost of needing to specify each of the nested directories in `source_entry_path`. Defining each namespace will make sure directories like `app/javascript/src`, `app/javascript/stylesheets`, etc don't get entries assigned.

* Move the entry parsing into `parseEntryPath` in `package/utils/helpers.js`
* Change `getEntryObject` to use `parseEntryPath` and to check whether `source_entry_path` is an array
* Add a nested pack into the test app
@@ -16,7 +16,7 @@ const getEntryObject = () => {
const entries = {}
const rootPath = join(config.source_path, config.source_entry_path)

globSync(`${rootPath}/**/*.*`).forEach((path) => {
globSync(`${rootPath}/*.*`).forEach((path) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ This change is huge for those who choose to upgrade to webpacker 6, leave their pack file in subdirectories and didn't want to change source_entry_path from packs to /

In that case,
// This behavior retained
app/javascript/packs/application.js

// Missing pack file
app/javascript/packs/foo/fooPack.js
app/javascript/packs/foo/fooPack.scss

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.

5 participants