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

Allow pack files to be placed in subdirectories #201

Merged
merged 1 commit into from
Apr 5, 2017

Conversation

kuroda
Copy link
Contributor

@kuroda kuroda commented Mar 29, 2017

With this commit, you can organize JavaScript files with namespaces.

For example:

app/javascript/packs/admin/hello_vue.js
app/javascript/packs/admin/hello.vue
app/javascript/packs/hello_vue.js
app/javascript/packs/hello.vue

@ronnyhaase
Copy link
Contributor

ronnyhaase commented Apr 4, 2017

I think not considering sub-folders is intended, because app/javascript/packs only contains your entry points and you should probably move the admin folder into app/javascript, and either import it in hello_vue.js or add a hello_vue_admin.js doing so.

I have no clue about Vue but e.g. for React this is totally fine.

@kuroda
Copy link
Contributor Author

kuroda commented Apr 5, 2017

@ronnyhaase

Thanks for your comment.

I suppose that you assume a kind of single page application with a very few entry points. Under this assumption, organizing entry points using subdirectories seems an overdo.

I personally, however, want to have a lot of entry points.

I often create a small Vue component in order to fullfil a small functionality, which used to be done with jQuery. In such case, there are times to need a separate vue file for each action.

Suppose that we have admin/users and portal/users resources, for example. I want to have a vue file for the admin/users#new action, and another for the portal/users#new action. In my opinion, it is very natural to put the former under the app/javascript/packs/admin directory, and the latter under the app/javascript/packs/portal directory.

This use case might not have been under consideration by the core team, but I think it is very important especially for Vue developers.

@ronnyhaase
Copy link
Contributor

I see your point, that you might have a requirement for subfolders in a huge app, where you have multiple areas like "admin" and "portal", each containing multiple pages and therefor entry points.

But I'm still not sure if Webpacker should go for that case by default, but this decision has to be made by the core contributors.

@p0wl
Copy link
Contributor

p0wl commented Apr 5, 2017

whats the downside of this pr? the app/javascript/packs folder is the root of entries of webpack and I don't see a reason why you would want subfolders in this directory if not for organizing entries.

So I think this pr enforces developers to keep the packs folder clean from anything but entry points, which I think is a good thing.

@dhh dhh merged commit 5e4f238 into rails:master Apr 5, 2017
@gauravtiwari
Copy link
Member

gauravtiwari commented Apr 5, 2017

@kuroda How about you just name space them within file name? For ex - admin-new or portal-new . I am not sure, but subdirectories seems redundant, when all we are keeping here are packs/entries, which is essentially going to be one per module or bundle.

Please correct me if I am assuming wrong?

This is what we have in readme,

// app/javascript/packs/calendar.js
require('calendar')

app/javascript/calendar/index.js // gets loaded by require('calendar')
app/javascript/calendar/components/grid.jsx
app/javascript/calendar/styles/grid.sass
app/javascript/calendar/models/month.js

If you have multiple namespaces, just name space the pack/entry name - admin-calendar.js, app-calendar.js.

I guess this change will create some edge cases

@dhh
Copy link
Member

dhh commented Apr 5, 2017 via email

@ronnyhaase
Copy link
Contributor

ronnyhaase commented Apr 5, 2017

I think there are arguments for both sides, but I agree with @dhh it fits with Rails namespaces. And thinking of bigger apps I would prefer directories instead of @gauravtiwari suggestion as well.

As an alternative option, how about only allowing one level of sub-directories by replacing the ** with a single * ? Just an idea, not sure if I like it.

@kuroda

@dhh
Copy link
Member

dhh commented Apr 5, 2017 via email

@kuroda
Copy link
Contributor Author

kuroda commented Apr 6, 2017

@gauravtiwari

Vue.js allows JS components to pick their template from an HTML element.

If we have this code as app/javascript/packs/hello_world:

import Vue from 'vue/dist/vue.esm'

document.addEventListener('DOMContentLoaded', () => {
  new Vue({
    el: "#app",
    data: { name: "world" },
    methods: {
      changeName: function() {
        this.name = "Alice"
      }
    }
  })
})

then, you can use this component like this:

<div id="app">
  <p>Hello, {{ name }}</p>
  <button v-on:click="changeName">Click me</button>
</div>
<%= javascript_pack_tag "hello_world" %>

With this approach, we can introduce Vue.js to an existing Rails app without massive code change.

I assume that this app has a lot of (hundreds of, maybe) pages which are organized using namespaces.
And we want to create a separate Vue.js component for each page.

In this circumstance, I don't want to see hundreds of JS files scattered in the app/javascript/packs directory.

@gauravtiwari
Copy link
Member

gauravtiwari commented Apr 6, 2017

Thank you, that totally makes sense and apologies for under-sight on my part. @dhh No, everything works (just tested it). I was worried that this might break the paths generated in the manifest :)

Thanks @kuroda 👍 🍰

robdimarco pushed a commit to robdimarco/webpacker that referenced this pull request May 24, 2017
…M error.

After upgrading to 2.0, I was consistently running out of RAM when trying to run webpack.

After some digging, I came across a comment on the webpack project that helped shed some light.

webpack/webpack#1914 (comment)

It seems that in PR 201 (rails#201), the `extensionGlob`
was changed from `*{${paths.extensions.join(',')}}*` to `**/*{${paths.extensions.join(',')}}*`

This change removes the `**/` in the extension and fixes my OOM issue.

I am not sure about the ramifications for allowing pack files in subdirectories
(the original intent of the PR that introduced the change). Maybe we need a comment or a
flag when generating the configuration that will set it one way or the other?
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