Aurelia adds /index to modules path after ModuleConcatenationPlugin

[homer0]

I'm submitting a bug report

• Library Version:
  3.0.0

Please tell us about your environment:

• Operating System:
  OSX 10.13.6

• Node Version:
  10.15.3

• NPM Version:
  6.4.1

• JSPM OR Webpack AND Version
  webpack 4.29.1

• Browser:
  all

• Language:
  ESNext

Current behavior:

When using .plugin(PLATFORM.moduleName('my/plugin/path')) on a production build, where ModuleConcatenationPlugin runs by default, the module ends up on the bundle with the key my/plugin/path/index, so the app fails to run as my/plugin/path doesn't exist.

Expected/desired behavior:

On the main configure of the app, use .plugin for a local folder index file that includes a component/service/valueConverter, basically anything that the ModuleConcatenationPlugin may want to optimize:

// main.js

export const configure = (aurelia) => {
  aurelia.use
  .plugin(PLATFORM.moduleName('modules/my-module'))
  .standardConfiguration();

  return aurelia.start().then(() => aurelia.setRoot(...));
};

// modules/my-module/index.js

import { MY_CONSTANT } = './my-constant.js';

export const configure = (aurelia) => {
  console.log(MY_CONSTANT);
};

// modules/my-module/my-constant.js

export const MY_CONSTANT = {
  someKey: 'someValue',
};

That's enough for ModuleConcatenationPlugin to try to put my-constant inside my-module/index.js and for the PreserveModuleNamePlugin to change modules/my-module to modules/my-module/index.

• What is the expected behavior?

For PreserveModuleNamePlugin not to add the /index.

• Possible solution

Not sure if this is the right way to do it, but if we add the following code before this line, it works:

if (id.endsWith('/index') && !realModule.rawRequest.endsWith('index')) {
  id = id.replace(/\/index$/, '');
  // or id = id.substr(0, id.length - 6);
}

I'm not that familiar with the module structure, so I'm not sure if we can "trust" in rawRequest

[jods4]

When using Aurelia with Webpack, you should use a path that refer to an actual JS module rather than a folder.

For a plugin local to your project, doing .plugin(PLATFORM.moduleName('modules/my-module/index')) should work.

Resolving index inside folders is a rule specific to the node/npm (although many bundlers will apply similar rules) and is tricky to get right at runtime in the context of Aurelia loader.

See the note in this section about how we changed the .feature() API (which appened /index implicitly) to avoid such issues.

[homer0]

@jods4 thank you so much for pointing that; after I found the issue and debugged it, I didn't think about looking on the wiki, I went to the issues and found a couple related to the ModuleConcatenationPlugin, but none of them about the index.

Now, I understand this correctly, the problem is not on either .feature nor .plugin, but the .moduleName right? if that's the case:

Wouldn't the fix I propose solve it? maybe with an extra condition just o be sure:

if (
  id.endsWith('/index') &&
  !realModule.rawRequest.endsWith('index') &&
  `${realModule.rawRequest}/index` === id
) {
  ...
}

If you think this can cause another problem, can you tell me the reason? I'm no very familiar with how the plugin does certain stuff but I would like to understand it a little bit better :D.

Also, in case that "fix" can't be implemented, wouldn't it be possible to add a warning log when .moduleName is called with a path for a directory that doesn't have a package.json? I believe I won't be the only one thinking that .moduleName uses the same resolution as Node's require (mostly because the syntax works until you make a production build).

It's kind of frustrating that the error doesn't give you a clue of what can be the reason for it; and then there's the fact that it only happens for some modules (the ones webpack wants to concatenate).

Thanks!

[jods4]

@homer0
I'm not 100% sure that change is safe in every situation.

Does it always work? If I have an index file inside an index folder it won't (I think that could be fixed with more complex code)

Does it work when Aurelia resolves relative modules at runtime? Probably not, because unlike Node, the browser runtime can't know if a name is supposed to be a folder or a file, so it always assumes it's a file. Removing /index will turn root/my-plugin/index in to root/my-plugin and then if the plugin references ./foo, Aurelia will look for root/foo instead of root/my-plugin/foo, which breaks.

This is actually the main reason why we changed the .feature() API when we added Webpack support.

Does it work well with dynamically built module names? e.g. data-driven <compose>?
Does it work well with aliases? etc.

I see where you're coming from and I agree that supporting this resolution logic would not be a bad thing.
The main issue is that this stuff is very tricky so before merging more resolution/normalizing rules, we have to make sure it doesn't create new issues or unexpected behavior for new users.

As it stands, I don't think this is safe to merge.

That said, when we can't make it work, I'm 100% in favor of detecting and warning users early. Ideally with a suggested fix.

1. Can you explain why it works in dev mode but breaks in prod? I don't understand precisely what happens in your case. Main difference between the two is which module names are preserved.

2. I would accept a PR that displays a warning when moduleName refers to a folder without package.json (i.e. Webpack will resolve to /index but it might not work in Aurelia). That warning should suggest refering to /index directly, as it's the fix.

[homer0]

@jods4 Hey! sorry I couldn't reply before, a loooot of work :P.

Thanks for the answer!

Does it always work? If I have an...

I understand this; in theory, we could add use fs to check if the path exists and if it's not a folder before changing it.

Does it work when Aurelia resolves relative modules at runtime? Probably not...

This I believe I don't understand; from what I could see, once the bundle is created, Aurelia doesn't care if it's a file or a directory, it's just the module ID on the webpack "container". This is actually how I discovered the first "symptom" of the issue: Aurelia was looking for modules/my-module and the key on the webpack container was modules/my-module/index.

Does it work well with dynamically built module names? e.g. data-driven...

Not sure about this, nor about the following question about aliases.

Now, regarding the other points:

1. The reason is breaks on prod is because when bundling there, webpack runs its optimizations, which include webpack's ModuleConcatenationPlugin, and as far as I understood it, when that plugins runs, this plugin's PreserveModuleNamePlugin runs... and that's when the problem happens.
2. I'll try my best to do some free time during this week/weekend and see if I can send you a PR.

Now, for the case of my app, I just disabled ModuleConcatenationPlugin as an optimization for production (it was saving just 2k and it's not a big app) and everything works perfectly... but I'm not using compose nor aliases, so I don't know if they would work.

[jods4]

@homer0 no problem, it's the same here... Overwhelmed by work it took me 3 weeks to get back at you.

This I believe I don't understand; from what I could see, once the bundle is created, Aurelia doesn't care if it's a file or a directory

You can (not saying you should) use relative modules. For example I commonly see features or plugin entries just being a list like this:

aurelia.globalResources([
  PLATFORM.moduleName('./autoFocus'),
  PLATFORM.moduleName('./data-tables'),
]);

Those relatives path will be resolved to full module names by Aurelia. At runtime it can't say if current module path is a file or folder. It always assumes the former and resolution will fail.

[homer0]

@jods4 Hey! no worries, same here.

I'm actually using that syntax for some components, but while they may be resolved by Aurelia, what I got from debugging the bundle is that PLATFORM.moduleName goes away and only the resolved path remains, which Aurelia uses that to require it, and it finally gets resolved by webpack's modules system.

Once again, thanks! and I'll try to make some time for the PR, sorry!

Edit: You can mark this as an "edge case" and close the issue :D.