search

apostrophecms / apostrophe

A full-featured, open-source content management framework built with Node.js that empowers organizations by combining in-context editing and headless architecture in a full-stack JS environment.
https://apostrophecms.com
MIT License
4.35k stars 591 forks source link

Should throw a hard error if a module has a slash in its name and is not following the npm namespacing convention #2646

Open jogarijo opened 3 years ago

jogarijo commented 3 years ago

To reproduce

Step by step instructions to reproduce the behavior:

  1. Pull a fresh version of A3 boilerplate: https://github.com/apostrophecms/a3-boilerplate/
  2. Create a module in a subdirectory, for example modules/nested/stuff/index.js: 
    module.exports = {
init: (self, options) => console.log(self.__meta.name, 'loaded')
}
  3. Declare said module in app.js 
    modules: {
...
'nested/stuff': {}
  4. Run the server
  5. Check the server output

Expected behavior

The module should be loaded without warning.

Describe the bug

At startup, the module nested/stuff is loaded and fully functional, but the warning below will appear:

You have a /path/to/your/project/modules/nested folder, but that module is not activated in app.js and it is not a base class of any other active module. Right now that code doesn't do anything.

Which doesn't make sense in this case, as nested is part of the name of the actual module, not a module by itself. At the same time, one can create nested modules that are not loaded without a warning being raised.

Details

Version of Node.js: 14 Apostrophe version used: 3.0.0-alpha.2

boutell commented 3 years ago

Actually arbitrary slashes are not allowed in module names. We should throw an error in this case to avoid confusion.

What we do allow is npm namespacing. That's a single parent folder starting with an @, which should ideally correspond to an npm namespace that you own; and the intended use of the feature, at project level, is to do project level configuration and overrides of a namespaced npm module. This also works in bundles.

jogarijo commented 3 years ago

Why should it be forbidden though? It grants more flexibility to organize the project modules, and it seems to be working with the current version without any change.

I tested the use of such nested modules for most common use cases, like relationship fields, widgets, pieces pages and so on. But now that I think about it, I didn't try to add browser scripts/styles in a nested module yet, and that may break assets generation somehow.

abea commented 3 years ago

2.x has nested module folders, but they need to be declared as such: https://docs.apostrophecms.org/core-concepts/modules/nested-module-folders.html. This isn't something we've got into for A3 yet, but maybe we should. Starting out more strictly seems fine since we can add more capabilities later. Organizing modules will be valuable, but the question is how we should go about it.

abea commented 3 years ago

And this seems less of a bug than a technical design question.

myovchev commented 3 years ago

I'd like to share my thoughts about this. I've used the nestedModuleSubdirs: true feature of a2 in my first project and I loved it! It really helps you organize your modules and scale your app gracefully. Additionally, when the time is right it's extremely easy to just move a sub-module to a npm module/bundle. So I strongly vote for porting this to a3. If you give me some directions I'm ready to help with it.

I'm all for keeping the current namespace requirement for local modules. It's a common thing (look at Java, PHP Symfony, etc worlds) and brings naming consistency (@mycompany, @myname) and again helps transition to (but it doesn't force to) namespaced npm module.

In summary - for local modules discovery, options should be nested module feature or namespaced nesting modules. It's simple and elegant.

boutell commented 3 years ago

This is what we already have in A2:

Note that project level modules may use an @ namespace, but it should be one you own in npm so that you can migrate those modules to public or private npm modules if you wish to, and will in any event not be in conflict with someone else's at any point.

Given where we are in the A3 release timeline it is likely that nestedModuleSubdirs will be ported over as-is if it is kept.

On Thu, Jan 21, 2021 at 2:32 AM Miro Yovchev notifications@github.com wrote:

I'd like to share my thoughts about this. I've used the nestedModuleSubdirs: true feature of a2 in my first project and I loved it! It really helps you organize your modules and scale your app gracefully. Additionally, when the time is right it's extremely easy to just move a sub-module to a npm module/bundle. So I strongly vote for porting this to a3. If you give me some directions I'm ready to help with it.

I'm all for keeping the current namespace requirement for local modules. It's a common thing (look at Java, PHP Symfony, etc worlds) and brings naming consistency (@mycompany https://github.com/mycompany, @Myname https://github.com/Myname) and again helps transition to (but it doesn't force to) namespaced npm module.

In summary - for local modules discovery, options should be nested module feature or namespaced nesting modules. It's simple and elegant.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/apostrophecms/apostrophe/issues/2646#issuecomment-764442428, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAH27J7SQZLRU4BXBDXFE3S27J7NANCNFSM4VHPBQFQ .

--

THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER APOSTROPHECMS | apostrophecms.com | he/him/his

boutell commented 3 years ago

The rules for parent folders starting with @ are not subject to change because we must be able to do project level overrides and configuration of namespaced npm modules. So that leaves only the question of what we do with parent folders that do not start with @. One feature we could add for parent folders starting with @ is support for the "index.js" feature that nestedModuleSubdirs parent folders have.

On Thu, Jan 21, 2021 at 7:33 AM Tom Boutell tom@apostrophecms.com wrote:

This is what we already have in A2:

  • If a parent folder in lib/modules starts with @, that is understood to be an npm namespace and treated as such, with module subdirectories inside it understood to be named @orgname/modulename.
  • Otherwise, it is invalid unless nestedModuleSubdirs: true is active.
  • If nestedModuleSubdirs is active, then parent dirs are treated as an organizational convenience and are not part of the name of a module.
  • nestedModuleSubdirs has one more feature: it allows you to configure the modules in an "index.js" file in the parent folder, which has the effect of breaking up app.js.
  • There is currently no such feature for @orgname parent folders.

Note that project level modules may use an @ namespace, but it should be one you own in npm so that you can migrate those modules to public or private npm modules if you wish to, and will in any event not be in conflict with someone else's at any point.

Given where we are in the A3 release timeline it is likely that nestedModuleSubdirs will be ported over as-is if it is kept.

On Thu, Jan 21, 2021 at 2:32 AM Miro Yovchev notifications@github.com wrote:

I'd like to share my thoughts about this. I've used the nestedModuleSubdirs: true feature of a2 in my first project and I loved it! It really helps you organize your modules and scale your app gracefully. Additionally, when the time is right it's extremely easy to just move a sub-module to a npm module/bundle. So I strongly vote for porting this to a3. If you give me some directions I'm ready to help with it.

I'm all for keeping the current namespace requirement for local modules. It's a common thing (look at Java, PHP Symfony, etc worlds) and brings naming consistency (@mycompany https://github.com/mycompany, @Myname https://github.com/Myname) and again helps transition to (but it doesn't force to) namespaced npm module.

In summary - for local modules discovery, options should be nested module feature or namespaced nesting modules. It's simple and elegant.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/apostrophecms/apostrophe/issues/2646#issuecomment-764442428, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAH27J7SQZLRU4BXBDXFE3S27J7NANCNFSM4VHPBQFQ .

--

THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER APOSTROPHECMS | apostrophecms.com | he/him/his

--

THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER APOSTROPHECMS | apostrophecms.com | he/him/his

myovchev commented 3 years ago

Just to be sure about it - @orgname/modulename as a local module (application level) without corresponding (installed) npm module should work fine?

boutell commented 3 years ago

It would work yes. You should register the corresponding npm organization so you have the option of publishing that module in future.

On Tue, Jan 26, 2021 at 8:57 AM Miro Yovchev notifications@github.com wrote:

Just to be sure about it - @orgname/modulename as a local module (application level) without corresponding (installed) npm module should work fine?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/apostrophecms/apostrophe/issues/2646#issuecomment-767557124, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAH27NDIY4HM3KFLYGHPNLS33C25ANCNFSM4VHPBQFQ .

--

THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER APOSTROPHECMS | apostrophecms.com | he/him/his

myovchev commented 3 years ago

It would work yes. :partying_face:

You should register the corresponding npm organization so you have the option of publishing that module in future. Yep, I'm aware of that. But it's good for every project as it could use something unique even without having claimed that npm namespace.