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

[pat841]

Reference: #122 @jods4 @EisenbergEffect Information:

/**
 *  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.
 */

[CLAassistant]

All committers have signed the CLA.

[jods4]

Thanks for creating this PR!

I feel bad about what I'm going to say but the timing is unfortunate... I just created branch v2.0-rc6, which contains lots of modifications to make the plugin compatible with Webpack 4. As you might know, Webpack 4 was just released last week and it contains breaking API changes that I had to fix.

Could you please rebase your changes on top of v2.0-rc6 and check that they work with Webpack 4? Also please don't use deprecated APIs that generate warnings. Hopefully you won't have too much to change and you can use my own fixes as reference.

[pat841]

@jods4 I went ahead and rebased but I have yet to test it as I have not yet upgraded my own build to webpack 4. If you have a working webpack 4 build, would you be able to test on your build for a reference against my webpack 3 build while I work on getting my build on webpack 4.

[rek72-zz]

@pat841 Hi Pat. This still shows open. Where are we at with this? Thanks.

[Alexander-Taran]

@pat841 have you had time to test your rebase?

[pat841]

@rek72 and @Alexander-Taran I have verified that this now works on both Windows, MacOS, and Linux using Webpack 4.

[rek72-zz]

Are you testing with the fixed version or what's been committed?

[jods4]

So I wanted to review this and possibly merge but it seems that it wasn't rebased properly. Somehow it contains one of my own commits e49fb8d, which (1) is already on master and (2) creates a lot of noise in the diff and makes it harder for me to review.

Can this be rebased on master please?

[pat841]

@jods4 I went ahead and re-based on master. Should be good to go.

[pat841]

@jods4

I went ahead and updated the PR, apologies on the delay. I commented with any questions I had and updated some things to conform to your style guide.

[pat841]

@jods4 Have you had a chance to review this yet?

[pat841]

Any update on this @jods4 @EisenbergEffect

[EisenbergEffect]

@jods4 Can you look into this?

[jods4]

I'm sorry I did not come back to this earlier. It needs a lot of focus and I've been working a little relentlessly those past months.

Thanks for the changes you've done so far, it's already easier to look at for me (less modifications).

The general changes I am fine with, modulo:

• 2 or 3 subtle behavior changes that might be unintended. I've pointed them out in comments, just clarify them for me. If they're unintended a revert should be quick.
• some style nitpicking, nothing important.

I like the change where you modify user code to inject the normalized module id instead of original one. I think that's a good change 👍

Then there's the node_modules stuff. It's a big change with lots of complicated new code. I'll try to read it again over the week-end, I'm too tired to do that now. I need to ensure that it works in many situations such as Windows vs linux (path manipulation), nested node_modules, symlinks... those are all common problems with node modules.

It would really help me a lot if you could summarize in a few word what use case you're trying to improve here. Can you provide an example of something that didn't work before and works with the new code?

I'm sorry it takes so long and is so hard to merge but that's a big change to the most tricky parts of this plugin.

[pat841]

@jods4 PR updated with changes.

As for WHY this PR is needed, you can see the original PR #122. This PR is a simplified version of that which fixes the original bug #121.

[jods4]

@pat841 I read issue #121 again and I'm confused. There is a huge change in this PR related to how node_modules is handled, yet the issue is all about how aliases are handled. Did I miss something? Or what did you change in node_modules that makes it better for aliases specifically?

[pat841]

@jods4 The issue was aliases were never handled properly when mixed with relative/absolute paths. This new approach creates a mapping of all normalized paths and replaces the alias/relative/absolute path in code with the normalized path.

Maybe we could tag this as beta/possible breaking and pre-release until were certain its stable? I have been using this PR in multiple projects of mine without issue .

[EisenbergEffect]

@pat841 We appreciate the awesome work you've put into this PR. One of the things that is holding this up is it's raw size. We usually like to work in smaller increments when we can. I wonder if there's a way that we could break this up into a set of successive, smaller PR, each addressing one issue. That would make the process go faster, it would be easier to review, test, etc.

Any thoughts on this? @bigopon and @fkleuver have done a good job of this on a few of our other libraries, so maybe they have some specific recommendations in this case.

[fkleuver]

I must admit that I often fail at breaking things up into smaller chunks as well. However, what I do to mitigate risks is add lots and lots of tests.

Tests are usually easier to review than code. You can look at simple inputs and outputs and verify whether they are appropriate, instead of having to reverse-engineer logic and guessing whether it might work or not.

So my recommendation would be to add tests. Whatever makes sense. A few unit tests could be good where possible (e.g. there's no need for a full project structure) and for the large areas it could be a full "e2e" tests on some real physical project structures where you have an input project and compare the processed output to a pre-generated one. Just to give an example. That way you can cover a lot of ground with relatively little work