moduleName() vs feature()
jods4 opened this issue · 10 comments
From my discussion with @EisenbergEffect I realized .feature() won't play along nicely with PLATFORM.moduleName(). Copy-pasting my description of the problem:
The idea so far was to use PLATFORM.moduleName('frob') to denote that the literal string 'frob' in code is actually a module name that is a direct dependency from the current file.
This is what the webpack build uses to properly trace all dependencies.
For users moving to this new system, the natural translation of .feature('frob') is going to be .feature(PLATFORM.moduleName('frob')).
But little will they realize that this documents a frob module dependency, when actually aurelia-loader is going to try to load frob/index.
This inconsistency (1) will not work; (2) can't be fixed in a pleasing way, even if you understand what happens.
Thoughts?
/CC @niieani
And some possible work-arounds:
- Add a parameter to add or remove
/indexfrom the module name, likemoduleName('frob', { resolve: 'frob/index' }). This needs good knowledge of Aurelia and is a bit ugly. - Embed special syntax in the string
moduleName('frob(/index)'). Similar problems, maybe less ugly. - Recognize
.feature()during parsing and add a special rule to automatically add/indexwhen resolving the dependency. Benefit: it works transparently. Drawbacks: added complexity in all tools that consume this; very non-intuitive behavior; this creates exceptions and special cases in the system. - Change
.feature()to optionally accept (and remove)/indexso that the following is valid:.feature(moduleName('frob/index')). This might be a breaking change if someone has a folder namedindex(unlikely). - Same as previous but introducing a new API to avoid the breaking change.
.featureFile(PLATFORM.moduleName('frob/index')). Bonus: could actually point to any file, not justindex.
How about we eventually deprecate and remove the .feature() API from Aurelia altogether?
I'm not sure it's really needed, as you can easily achieve the same with one import/function usage (within main.js), and requires no Aurelia-specific knowledge.
Is there a specific reason for even having .feature()?
CC: @EisenbergEffect
No, feature has other purposes. For example, it provides a consistent way to enable code to run configuration and setup after the core of Aurelia has started up, but before the application is rendered. Additionally, feature/plugin code can be async and the framework just handles that. This allows you to build portions of your application the same way you would external plugins and makes it easy to migrate from a feature to a plugin and vice versa. But, additionally, there are more capabilities planned for this in the future, such as lazy features. The idea there is that you use the feature or plugin API to setup a feature, but that the load is actually delayed until later. Then, when the functionality within that feature is needed, just in time, the feature modules are loaded, their configuration is run asynchronously and then the feature becomes available for use. That's a long time request from the community for both features and plugins that we'll want to support. It's not too much work to enable it really. We just have had other more important things to do first.
OK, thanks for clearing this up @EisenbergEffect. Sounds like a nice feature indeed, I'm not sure it was ever documented in a way which tackles the above notions (or I simply missed that). I guess we need to think of a way to support these scenarios given the use-cases you presented.
The lazy part isn't implemented anywhere. I think adding that is where it would get interesting.
@EisenbergEffect We need to decide how we want to proceed with this. The present workaround is not pretty.
If we agree on adding featureIndex() that takes the file directly (not necessarily named index btw) I can open a PR.
Why can't we keep the current method and just update the implementation to check for strings that end with /index? If it has that ending, it doesn't alter it. If it doesn't have that ending, it adds it like it does today.
Do you mean update the current .feature() implementation?
Yes, that's a possibility (second to last in my bullet list).
So the migration is .feature('abc') -> .feature(moduleName('abc/index')).
Note that this is a breaking change if you have named your folder index and that the only work-around is terrible: index/index/index is translated to index/index which would then work. It's highly unlikely, but just sayin'.
Yes, update the current feature implementation. I think it's unlikely that someone has a folder named index. We could detect that potentially and issue a meaningful error message. I really doubt this will be a problem. We might want to put a deprecation info log in to get the community to start moving everything over to using index explicitly as well.
Closing this issue as we merged aurelia/framework#710, which adds support for .feature('folder/index').