fix(webpack): Aliased module paths now properly map to the correct aurelia-loader module id

[pat841]

Fixes the issues I ran into here: #121

The routes below will now correctly render:

      // Not working?
      // { route: ['', 'sample'], name: 'sample', moduleId: PLATFORM.moduleName('test_alias/sample'), title: 'Sample' }
      // { route: ['', 'sample'], name: 'sample', moduleId: PLATFORM.moduleName('test_alias/test/testsample'), title: 'Sample' }
      // { route: ['', 'sample'], name: 'sample', moduleId: PLATFORM.moduleName('test_alias/test_feature/testfeaturesample'), title: 'Sample' }
      // { route: ['', 'sample'], name: 'sample', moduleId: PLATFORM.moduleName('test_alias/test_plugin/testpluginsample'), title: 'Sample' }

[CLAassistant]

All committers have signed the CLA.

[jods4]

Thanks for that contribution! Working with Webpack's internals is not the most easy part :)

It's complex, so we'll probably have to do more than one pass... I just did the first one!

In addition to the code comments, I would appreciate a short summary of your thinking / strategy.

Today I support aliases to refer to files outside your webpack modules folders.

You want to add support for using aliases that point inside your modules folder.
Can you briefly explain your design, how it will work?
In particular, you now have multiple ways of expressing the same paths. Possibly multiple modules refer to a single module with different paths (one using an alias, or another, or no alias at all). What is the expect behavior in this case?

[jods4]

getPreservedModules is not really the good function for that. It's goal is to return the list of modules that must have their ids preserved.

Finding good ids for those modules starts later, on line 39.
There are several variations, you can add a new one, or maybe see if yours fit into aliasRelative, which already exists (but takes lower precedence than a path in a modules folder).

[jods4]

That has to be wrong. You set the module id to a name that comes from another module, modulo aliases resolution?!
If it's correct there's something I don't understand here.

[jods4]

Is the regexp totally safe? Did you consider if aliasPath might need escaping? For example, what if it contains dots or dollars?
Be careful that Webpack also has a convention where an alias ending with $ must be an exact match (which is what it would mean in a regexp, so that's ok at least).

[jods4]

So you're breaking the compilation if more than one alias could have produced a path?
That seems wrong. For instance, I should be able to define all the aliases I want, as long as I don't use them there's no reason that the compilation should fail.

[jods4]

If you're not interested in the results, just whether the regex matches, using .search() is more self explanatory.
All RE functions like match, exec or search can take a string, no need to new RegExp in there.

[jods4]

Did you consider that you're in a loop over the possibly many reasons that a module was included in the compilation?

Such reasons might not be related to Aurelia at all, like regular ES import. Is it correct to consider them?
What if multiple reasons lead to conflicting uses of different aliases or absence of aliases?

[pat841]

@jods4

Pretty big overhaul. When we were generating module ids based on its request I kept running into conflicts when aliased modules were referencing relative modules and other common scenarios.

Instead, I am now generating module ids based on the resource location. The only tricky parts I ran into were detecting the exported modules and entry point for modules existing in node_modules. I commented the cases pretty thoroughly and the process I use to detect the entry point.

I have been playing around with this PR all day and haven't ran into issues when using absolutes, relative, and aliased paths in PLATFORM.moduleName().

[pat841]

Major overhaul. Now using module resource rather than its request.

[atsu85]

@pat841, please revert the changes in generated files, that are updated by release process

[jods4]

@pat841 I tried to review the changes today but the "major overhaul" was a little too major to easily see what changed (basically the whole file is in the diff).

I want to be very careful here as this is the probably the most tricky part of the plugin and there are tons of cases.

The changes to support internal aliases are drawn into many other changes such as code style changes, refactorings and added jsdocs.

Could you please split this PR into 2:

• keep this one for the internal aliases feature.
• maybe start a PR -- or better a discussion first -- to refactor code. Some of your changes are good, other less so, but overall consistency is important and modifying a single file like that makes it really stand out.

Focusing on the internal alias:
I asked before to describe in words your design for the feature.
I would like to discuss the possible impacts to understand them well (and modify code/docs if needed).

Without reading your design, some questions that come to my mind:

• What happens if someone has an internal alias, but sometimes refers to modules inside that path without using the alias? Does it work, if not do we have a good error, is it fixable, can we just ignore it?
• What happens if aliases overlap for a given path? Do we take the longer one, does it matter, can we ignore it?
• Is there a chance that a working build today is broken by this change?
• Can we combine this with the existing support for external aliases?

[jods4]

@pat841 I've been thinking about this a little more.

One scenario that nags me is what happens if you have an internal alias, say aka = x/y/ and then a module mx inside x imports ./y/my while another module somewhere else imports aka/my.

With your alias probing, my will normalize to aka/my, which is fine for the second import. But the ./y/my import from x won't work because Aurelia will search for x/y/my, unaware of the compile-time alias.

Not sure how to frame this so that it's robust and without surprises :(

[pat841]

@jods4 I can create another "lesser overhaul" PR if needed, it could take me a bit though. But overall I sorta just broke certain sections apart into smaller functions.

That majority of the changes are in:

• In getPreservedModules(), rather than setting the module id in the loop before, we just filter for modules that need to be preserved instead.
• Resolving node_modules/, look at the docs in parseNodeModules(). Before we relied on how the module was being requested, now we look at where it actually lives relative to node_modules/ and map that way.
• In the AureliaDependencyPlugin, rather than using the request for preserved modules, we use the module id that was generated earlier and prevents conflicts in the webpack loader.

As for your previous scenario, there wouldnt be an issue. We probe node_module -> resolver paths -> aliased paths -> error in that order. Meaning both import ./y/my and import aka/my would translate the import path to y/my.

[jods4]

Both [...] would translate the resolve to ./y/my

Not sure I understand.
./y/my cannot be a module id, because it's relative. aurelia-loader always resolves all requests to an absolute module id.
What did you mean by "translate the resolve"?

[pat841]

@jods4 Apologies, I edited my post. Each PLATFORM.moduleName() import (relative, absolute, or aliased) resolves to a webpack dependency. Once every module dependency has been resolved via webpack, we go through each module imported (via PLATFORM.moduleName()) and build an absolute module id. Once we each module has a generated id, we then replace the old import (relative, absolute, aliased) with the absolute module id.

No matter if a module is referenced via a relative, absolute, or aliased path in any number of files, webpack is going to resolve it to a single module.

[jods4]

No matter if a module is referenced via a relative, absolute, or aliased path in any number of files, webpack is going to resolve it to a single module

Yes, but then there's the runtime resolution by aurelia-loader and it should match the module ids that we assign.

From my scenario above, a request for ./y/my inside x/ will be normalized by aurelia-loader to x/y/my, which from your example would not exist because its module id would be aka/my.

[pat841]

@jods4 No, because we replace the old ./y/my and aka/my imports with the same webpack module id which would be y/my.

@useView(PLATFORM.moduleName('./y/my');
---> @useView(PLATFORM.moduleName('y/my');

@useView(PLATFORM.moduleName('aka/my');
---> @useView(PLATFORM.moduleName('y/my');

[jods4]

I did not implement modifying the source js, did you?

It's tempting as it solves a lot of issues with module ids normalization, but it won't work flawlessly because there are various ids we don't control (and can't replace). For example ids that are derived by conventions (e.g. views) or that are built dynamically by user code and bound into <compose> and co.

[pat841]

Yeah, thats what I added. We only replace if its a module that is dynamically imported.

class Template {
  apply(dep: AureliaDependency, source: Webpack.Source) {
    // Get the module id, fallback to using the module request
    let moduleId: string = dep.request;
    if (dep.module && typeof dep.module[preserveModuleName] === 'string') {
      moduleId = dep.module[preserveModuleName];
    }

    source.replace(dep.range[0], dep.range[1] - 1, "'" + moduleId.replace(/^async(?:\?[^!]*)?!/, "") + "'");
  };
}

Ive tested this change on the skeletons and my own repos without issue so far. You can test yourself using: "aurelia-webpack-plugin": "git://github.com/pat841/webpack-plugin.git#dist"

[jods4]

Transforming PLATFORM.moduleName('xyz') into the actual Webpack module id '42' or even any other id (path-like?) we would assign to a module is an idea that was very appealing to me.

The awsome thing is that it would guarantee that runtime loads never fail because the resolution is different from Aurelia and we'd get rid of all those shitty problems people with special setups sometimes have.

But modifying ids in JS code is an absolute can of worms that I don't want to open and I think that you'll have a hard time convincing me otherwise (but go ahead, I would love to change my mind about this).

Here is a list of problems that need to be solved, I'm probably missing some:

It only works for moduleName strings. Unfortunately there are module names that are constructed differently:

1. By (user-customizable) conventions, e.g. finding the view that goes along a viewmodel.
2. Sometimes people build dynamic module ids by mixing strings in their models and then do something like <compose view.bind="someProperty">. To ensure the views are included in the build, various strategies can be used, e.g. GlobDependenciesPlugin.
3. There are some weird code patterns where Aurelia wants to discover a VM's origin and then resolve resources relative to it (or find a View) although the VM has never been loaded through aurelia-loader but with import instead. This is already quite a problem, typical use case is a VM for aurelia-dialog.
4. Another issue is that this will break people's code if they forget moduleName somewhere in their codebase. Let's say they have one moduleName('mod-abc') somewhere that brings mod-abc into the mix, but somewhere else in code they just have 'mod-abc'. As long as the module ids are correct the app will work at runtime and that's rather a good thing. When transforming ids in JS code, there will be a mismatch and the code won't work anymore.
5. Which brings me to debugging. The current format is rather convenient when you have problem... you can still (mostly) figure out what happens. Depending on the ids we give, maybe not as much.
6. Keeping names and correct path structure seems mandatory when producing Webpack DLLs.
7. Finally, this adds more complexity to the Aurelia plugin, something I'd rather avoid unless it pays for itself in a compelling way.

Those reasons are why the current strategy is to produce module ids that match what aurelia-loader resolves at runtime. Fundamentally, this is how Aurelia works with other loaders, e.g. AMD.

[pat841]

@jods4 Would it be beneficial to setup a base project that includes any and all possible import cases?

[jods4]

@pat841 That would be a nice test bench for the plugin, it would be a hell of a project, though! Not sure if you even can fit everything into a single project, maybe 2 or 3...

I'm traveling for three weeks starting now, so I won't be able to look at this for a while, don't worry that's normal ;)

[EisenbergEffect]

@jods4 Where are we at with this?

[pat841]

@EisenbergEffect @jods4 Ive been using this rev for over a couple months now without issue using aliased, absolute and relatives paths interchangeably throughout my project.

[jods4]

@EisenbergEffect @pat841 Above, I have expressed some concerns about this PR. Interesting discussions such as modifying JS source ensued but I don't think any of my concerns has been addressed.

This stuff is unfortunately the trickiest part of the plugin and during the first RCs, lots of weird use cases have popped up, which I did my best to support. The fact that it works in one project is far from enough to merge this blindly.

As I said above, I would like to understand clearly what the impact is.

Currently the diff is a full file change because of many style/typings/comments changes that are not relevant to the internal alias support. I suggested that this PR was split in 2 PRs to make it easier to review each one.

I would like a conceptual description of how the new internal alias support works.

Finally, I asked a few questions about edge-cases in this comment and the next one.

[pat841]

@jods4 @EisenbergEffect

I created a PR with a minimal a change as I could, and a bit more documentation: #139

/**
 *  The purpose of this plugin is to track down where exactly each included dependency lives and build a module
 *  name from that. Since projects and webpack configurations can be vary, we do our best to guess but expect edge-cases
 *  to be hit and changes needed.
 *
 *  Process:
 *  - Each AureliaDependency contains the preserveModuleName symbol to notify this plugin to track the dependency
 *  - AureliaDependenciesPlugin searches and includes all PLATFORM.moduleName() dependencies  and stores as
 *    an AureliaDependency
 *  - ConventionDependenciesPlugin searches for all relative includes and stores as an AureliaDependency
 *  - At this point, webpack has resolved all included modules regardless of using a relative or absolute path
 *  - Now we want to normalize each include so that we can reliably replace the include string to match the webpacks
 *    module id
 *  - We then unwrap all the modules from ModuleConcatenationPlugin to get the raw dependencies
 *  - For each raw dependency that is included via node_modules/, read from its location:
 *      - Example Path: /home/usr/pkg/node_modules/mod/dir/file.js
 *          - The path to the module itself: /home/usr/pkg/node_modules
 *          - The module name: mod
 *          - The relative path: dir/file.js
 *      - Map the parsed path data to _nodeModuleResourcesMap by the parsed module name and its resource location
 *          - Example Map:
 *              {
 *                'aurelia-templating-router': {
 *                  '/home/usr/pkg/node_modules/aurelia-templating-router/dist/native-modules/router-view.js': {
 *                    path: '/home/usr/pkg/node_modules/aurelia-templating-router',
 *                    name: 'aurelia-templating-router',
 *                    relative: '/dist/native-modules/router-view.js',
 *                  },
 *                  '/home/usr/pkg/node_modules/aurelia-templating-router/dist/native-modules/route-href.js': {
 *                    path: '/home/usr/pkg/node_modules/aurelia-templating-router',
 *                    name: 'aurelia-templating-router',
 *                    relative: '/dist/native-modules/route-href.js',
 *                  },
 *                  '/home/usr/pkg/node_modules/aurelia-templating-router/dist/native-modules/aurelia-templating-router.js': {
 *                    path: '/home/usr/pkg/node_modules/aurelia-templating-router',
 *                    name: 'aurelia-templating-router',
 *                    relative: '/dist/native-modules/aurelia-templating-router.js',
 *                  },
 *                  '/home/usr/pkg/node_modules/aurelia-templating-router/dist/native-modules/route-loader.js': {
 *                    path: '/home/usr/pkg/node_modules/aurelia-templating-router',
 *                    name: 'aurelia-templating-router',
 *                    relative: '/dist/native-modules/route-loader.js',
 *                  },
 *                }
 *              }
 *  - For each mapped node_module, look at the included resources and normalize:
 *      - Look at the modules included resources and find a common path and its entry point
 *          - Entry point conditions:
 *              - Resource name matches 'index'
 *              OR Resource name matches the module name
 *              OR It is the only included module resource
 *          - If there are multiple entry points:
 *              - Pick the most shallow resource
 *              - If they are both as shallow as possible choose index over module name match
 *      - Map the normalized module id to _nodeModuleResourceIdMap by the resource file
 *          - Example Map:
 *              {
 *                '/home/usr/pkg/node_modules/aurelia-templating-router/dist/native-modules/router-view.js': 'aurelia-templating-router/router-view.js',
 *                '/home/usr/pkg/node_modules/aurelia-templating-router/dist/native-modules/route-href.js': 'aurelia-templating-router/route-href.js',
 *                '/home/usr/pkg/node_modules/aurelia-templating-router/dist/native-modules/aurelia-templating-router.js': 'aurelia-templating-router',
 *                '/home/usr/pkg/node_modules/aurelia-templating-router/dist/native-modules/route-loader.js': 'aurelia-templating-router/route-loader.js',
 *              }
 *  - Look at all webpack modules and for each module that includes or has a dependency that includes an AureliaDependency:
 *      - Handling module ids can be a bit tricky. Modules can be included in any of the following ways:
 *          import { Module } from 'module'
 *                                 'module/submodule'
 *                                 './module'
 *                                 'folder/module'
 *                                 'alias/folder/module'
 *                                 'alias$'
 *                                 '@scope/module'
 *          @decorator(PLATFORM.moduleName('module'))
 *                                 ...
 *      - The problem arises when aurelia-loader has to know the module to use at runtime. Webpack Mappings:
 *          - Absolute Module: 'module' -> 'module'
 *          - Relative Module: './module' -> 'folder/module'
 *          - Absolute Path: 'folder/module' -> 'folder/module'
 *          - Aliased Path: 'alias/folder/module' -> 'alias/folder/module'
 *          - Absolute Alias Path: 'alias$' -> 'alias$'
 *      - In order to have the aurelia-loader work correctly, we need to coerce everything to normalized ids
 *          - If the module is in the node_module/ map, use the normalized module id
 *          - If the module exists inside a webpack resolver path, use the relative path as the module id
 *          - If the module exists inside a webpack alias path, use the relative path from the alias path as the module id
 *      - Set the modules preserveModuleName Symbol export so the AureliaDependenciesPlugin can read it later
 *  - In AureliaDependenciesPlugin, for each AureliaDependency, replace the original and now incorrect PLATFORM.moduleName()
 *    path with the normalized path.
 *  - Source files now contain the normalized module paths with map correctly to the webpack module ids so that the
 *    aurelia-loader can find them during runtime.
 */

[jods4]

@pat841 thanks! Does it replace this one? I mean: should we close #122?

[pat841]

@jods4 This includes the additional typings minus the header docs so its up to you, I can make the needed changes.

[jods4]

But we can't merge both PR because they contain mostly the same changes, right?