Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
from

Conversation

@pat841
Copy link

pat841 commented Feb 26, 2018

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

This comment has been minimized.

Copy link

CLAassistant commented Feb 26, 2018

CLA assistant check
All committers have signed the CLA.

@jods4

This comment has been minimized.

Copy link
Contributor

jods4 commented Feb 26, 2018

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 pat841 force-pushed the pat841:simple branch from fe5933d to e48aaff Feb 26, 2018
@pat841

This comment has been minimized.

Copy link
Author

pat841 commented Feb 28, 2018

@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

This comment has been minimized.

Copy link

rek72 commented Jun 12, 2018

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

@Alexander-Taran

This comment has been minimized.

Copy link
Member

Alexander-Taran commented Jun 13, 2018

@pat841 have you had time to test your rebase?

@pat841

This comment has been minimized.

Copy link
Author

pat841 commented Jun 14, 2018

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

@rek72

This comment has been minimized.

Copy link

rek72 commented Jun 14, 2018

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

@jods4

This comment has been minimized.

Copy link
Contributor

jods4 commented Jun 16, 2018

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 pat841 force-pushed the pat841:simple branch 3 times, most recently from 15eb138 to 568dc37 Jun 26, 2018
@pat841

This comment has been minimized.

Copy link
Author

pat841 commented Jun 26, 2018

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

@pat841 pat841 force-pushed the pat841:simple branch from 568dc37 to 146dbc8 Jun 26, 2018
Copy link
Contributor

jods4 left a comment

This is a huge PR to the most complex part of this plugin so it'll take some time for me to review entirely.

First pass is mostly coding style...

I'm trying to get a good grasp of the modifications. Can you provide a quick summary of what behavior you intended to change with this PR? What specific use-case works differently than before?

return !!value;

// Preserve the module if its dependencies are also preserved
const reasons = (m.reasons && Array.isArray(m.reasons)) ? m.reasons : [];

This comment has been minimized.

Copy link
@jods4

jods4 Jul 2, 2018

Contributor

Is there a reason for this defensive code?
It was not in my code and I've never had an issue filed about m.reasons not being an array.
Is there a case where it happens?

This comment has been minimized.

Copy link
@pat841

pat841 Aug 27, 2018

Author

I am unsure, I usually add safety checks and assume to worst. If you need me to I can remove it.

This comment has been minimized.

Copy link
@jods4

jods4 Oct 11, 2018

Contributor

The problem I have with defensive code is that it creates headache for future maintainers.

When I see this code it tells me "m.reasons could be something else than an array, we better be careful" and I immediately ask myself: "how so? what am I missing?".

If this m.reasons can be null we should document how / why, it's useful information.
If it cannot, then it's misleading bloat that I'd rather remove.

When unsure, someone needs to figure this out and it's courteous that you do it rather than deferring the analysis to future maintainers.


// Preserve the module if its dependencies are also preserved
const reasons = (m.reasons && Array.isArray(m.reasons)) ? m.reasons : [];
return reasons.some((reason) => Boolean(reason.dependency && reason.dependency[preserveModuleName]));

This comment has been minimized.

Copy link
@jods4

jods4 Jul 2, 2018

Contributor

Nit: no need for parenthesis when there's a single argument. No need to convert to bool explicitely as it's done by some.

@@ -57,11 +147,13 @@ export class PreserveModuleNamePlugin {
if (/^async[?!]/.test(realModule.rawRequest))
id = "async!" + id;

id = id.replace(/\\/g, "/");
id = id.replace(/\\/g, '/');

This comment has been minimized.

Copy link
@jods4

jods4 Jul 2, 2018

Contributor

All strings use double-quotes, why did you change this one to single-quote?
Also: consistency is good, make sure your new code uses double-quotes (I spotted some single-quotes).

* @return {string|null} The module id if available, null otherwise
*/
function getModuleId(module: Webpack.Module, paths: string[], aliases: { [key: string]: string } | null): string | null {
if (!module) {

This comment has been minimized.

Copy link
@jods4

jods4 Jul 2, 2018

Contributor

Why? This is an internal API and you've typed module: Webpack.Module.
We compile with strict null checks enabled, so this is not possible.

aliasRelative(alias, realModule.resource);

// Get the module id
let id = getModuleId(realModule, roots, alias);

This comment has been minimized.

Copy link
@jods4

jods4 Jul 2, 2018

Contributor

If we factor out getModuleId, we might as well take the if (!id) throw below with it.
That changes the return type to a non-null value.

This comment has been minimized.

Copy link
@pat841

pat841 Aug 27, 2018

Author

What do you mean by this?

This comment has been minimized.

Copy link
@jods4

jods4 Oct 11, 2018

Contributor

I mean on next line (140) you throw if the id is null.
You can make getModuleId throw if it can't find an id; and have a non-nullable return.

Non-nulls are better coding style and it makes getModuleId easier to re-use by not having to check its result. Mainline code is simpler as well.

// 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];

This comment has been minimized.

Copy link
@jods4

jods4 Jul 2, 2018

Contributor

I am not sure what the impact of this change is.
Can you explain briefly, maybe with an example that behaves differently before/after this change?

This comment has been minimized.

Copy link
@pat841

pat841 Aug 27, 2018

Author

AureliaDependenciesPlugin uses the preserveModuleName symbol to set the generated moduleId. We then check that it is set here and use that for the webpack moduleId, otherwise fallback to the default usage.

This comment has been minimized.

Copy link
@jods4

jods4 Oct 11, 2018

Contributor

OK, I see.
I had this idea of rewriting user code with Webpack generated id but it breaks quite a few things (such as conventions).

You take a middle ground where you rewrite user's code with our normalized id.
That seems safe and would solve a few issue, I think, as aurelia-loader will do the same normalization anyway.., and in the rare cases where it doesn't (there are a few) it would break at runtime anyway.

Looks like a good change to me 👍

Out of curiosity: does this fix a specific problem you have?

This comment has been minimized.

Copy link
@pat841

pat841 Oct 16, 2018

Author

Yes it does see #121

@@ -41,7 +48,7 @@ class ParserPlugin {
hooks.evaluateIdentifier.tap("imported var.moduleName", TAP_NAME, (expr: Webpack.MemberExpression) => {
if (expr.property.name === "moduleName" &&
expr.object.name === "PLATFORM" &&
expr.object.type === "Identifier") {
String(expr.object.type) === "Identifier") {

This comment has been minimized.

Copy link
@jods4

jods4 Jul 2, 2018

Contributor

Why? What can expr.object.type be that is not a string but would convert to "Identifier"?

This comment has been minimized.

Copy link
@jods4

jods4 Oct 11, 2018

Contributor

OK, I see now that newer TS shows an error here.
A cast is a hack, the real problem lies in my poor type definition of expr.object.
Would be nice to fix it there instead.

Same for the later occurances of String(expr.object.type)

@@ -55,8 +62,8 @@ class ParserPlugin {
// PLATFORM.moduleName("id");
hooks.evaluate.tap("MemberExpression", TAP_NAME, expr => {
if (expr.property.name === "moduleName" &&
(expr.object.type === "MemberExpression" && expr.object.property.name === "PLATFORM" ||
expr.object.type === "Identifier" && expr.object.name === "PLATFORM")) {
(String(expr.object.type) === "MemberExpression" && expr.object.property.name === "PLATFORM" ||

This comment has been minimized.

Copy link
@jods4

jods4 Jul 2, 2018

Contributor

Same question here and on next line: what can expr.object.type be that is not a string and would convert to "MemberExpression"?

* @return {string|null} The escaped string
*/
function escapeString(str: string): string | null {
if (typeof str !== 'string') {

This comment has been minimized.

Copy link
@jods4

jods4 Jul 2, 2018

Contributor

internal API with type string...

*
* @return {string|null} The module id if available, null otherwise
*/
function getModuleId(module: Webpack.Module, paths: string[], aliases: { [key: string]: string } | null): string | null {

This comment has been minimized.

Copy link
@jods4

jods4 Jul 2, 2018

Contributor

When extracting this function, we lost one bit of functionality.
If module[preserveModuleNode] was defined it would be used without looking at anything else.
This can be set by some plugins when creating the dependency, if I recall correctly.

Is this intentionnal?

This comment has been minimized.

Copy link
@pat841

pat841 Aug 27, 2018

Author

I was unaware that other modules could set it. The AureliaDependenciesPlugin uses it to explicitly set the generated moduleId.

This comment has been minimized.

Copy link
@jods4

jods4 Oct 11, 2018

Contributor

See for example (with some comments on why) ConventionDependenciesPlugin

@pat841

This comment has been minimized.

Copy link
Author

pat841 commented Aug 27, 2018

@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

This comment has been minimized.

Copy link
Author

pat841 commented Sep 26, 2018

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

@pat841

This comment has been minimized.

Copy link
Author

pat841 commented Oct 10, 2018

Any update on this @jods4 @EisenbergEffect

@EisenbergEffect

This comment has been minimized.

Copy link
Member

EisenbergEffect commented Oct 11, 2018

@jods4 Can you look into this?

@@ -58,7 +58,7 @@ declare namespace Webpack {
// Those types are not correct, but that's enough to compile this project
property: IdentifierExpression;
object: { name: string; type: string; } & MemberExpression;
type: "MemberExpression";
type: "MemberExpression" | "Identifier";

This comment has been minimized.

Copy link
@jods4

jods4 Oct 11, 2018

Contributor

This seems very wrong MemberExpression is defined by type: "MemberExpression", it can't be Identifier.
The problem is rather on previous line where object: { ... } & MemberExpression;

I noticed that newer TS version create error because of this, we need to fix it.
I'm not sure why I wrote & MemberExpression in the first place here, because it's obviously not 100% correct.

export const preserveModuleName = Symbol();

// node_module maps
const _nodeModuleResourcesMap: NodeModule.ResourcesMap = {};
const _nodeModuleResourceIdMap: NodeModule.ResourceIdMap = {};

This comment has been minimized.

Copy link
@jods4

jods4 Oct 11, 2018

Contributor

nit (style): other private members are not _ prefixed in this project.

@@ -30,6 +123,10 @@ export class PreserveModuleNamePlugin {
modulesBeforeConcat.splice(i--, 1, ...m["modules"]);
}

// Map and parse the modules if needed
modulesBeforeConcat.forEach((m) => mapNodeModule(m));

This comment has been minimized.

Copy link
@jods4

jods4 Oct 11, 2018

Contributor

nit: modulesBeforeConcat.forEach(mapNodeModule), no need for lambda.

return true;
}
if (m[preserveModuleName]) {
return true;

This comment has been minimized.

Copy link
@jods4

jods4 Oct 11, 2018

Contributor

It seems to me that you have significantly changed the behavior of this.
We did not just check if some dependency had preserveModuleName set.

If the dependency used an absolute module id, we would use it as a module name, as this is more robust than trying to figure out the normalized id ourselves. See line 82 before PR.

Unless you have a good reason to remove that behavior, I would like to revert this change.

This comment has been minimized.

Copy link
@pat841

pat841 Oct 16, 2018

Author

I am confused by this. Why would we set a modules id based on one of its dependencies? ModuleA could require ModuleB but we dont want to set ModuleA's id based on ModuleB which might or might not live in the same module directory.

This comment has been minimized.

Copy link
@jods4

jods4 Oct 17, 2018

Contributor

Well I said it here:
If the dependency used an absolute module id, we would use it as a module name, as this is more robust than trying to figure out the normalized id ourselves.

Not sure how to say it better.
Normalizing the module names for aurelia-loader is a can of worms. It works rather well but there are lots of edge cases, this PR is proof of that.
So when we are handed out an absolute module id, that's great because it means we don't have to make guesses at normalization and it's guaranteed to work.
It's also marginally faster.

@jods4

This comment has been minimized.

Copy link
Contributor

jods4 commented Oct 11, 2018

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 pat841 force-pushed the pat841:simple branch 2 times, most recently from 1e634ff to 103d9b7 Oct 16, 2018
@pat841 pat841 force-pushed the pat841:simple branch from 103d9b7 to 219ae08 Oct 16, 2018
@pat841

This comment has been minimized.

Copy link
Author

pat841 commented Oct 16, 2018

@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.

@pat841 pat841 force-pushed the pat841:simple branch from 219ae08 to 5426b42 Oct 16, 2018
@jods4

This comment has been minimized.

Copy link
Contributor

jods4 commented Oct 17, 2018

@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

This comment has been minimized.

Copy link
Author

pat841 commented Feb 20, 2019

@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

This comment has been minimized.

Copy link
Member

EisenbergEffect commented Feb 27, 2019

@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

This comment has been minimized.

Copy link
Member

fkleuver commented Feb 28, 2019

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

…relia-loader module id [SIMPLE]
@pat841 pat841 force-pushed the pat841:simple branch from 5426b42 to 2299267 Nov 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.