Skip to content
This repository has been archived by the owner on Mar 6, 2024. It is now read-only.

Fix route-analyzer not handling equal module basenames #482

Conversation

kepelrs
Copy link

@kepelrs kepelrs commented Jan 17, 2023

Before this commit, the route-analyzer would search for modules by their basenames (eg. my-module.ts). This could match the wrong module in cases where we had two module files with same file name but in different folders.

This change fixes the issue by matching absolute paths instead of only basenames.

Signed-off-by: Davi Barreto [email protected]

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Examples have been added / updated (for bug fixes / features)
  • Changelog has been updated

PR Type

What kind of change does this PR introduce?

  • Bugfix

What does this change do?

This change makes the route-analyzer correctly handle duplicate file names by using the absolute path.

What manual testing did you do?

Test new output is unchanged when there are no duplicate module names

a. Run the old route-analyzer locally against VCD master
b. Store the contents of `gen-routes/tenant/app-route.ts` into a separate file
c. Run the newly changed route-analyzer locally against VCD
d. Verify the generated `app-route.ts` file from step a. is strictly equal to step c.

Test the old script breaks when duplicate modules are found

a. checkout vcd_ui master
b. cd content/core
c. Run the following commands to setup a duplicate module with loadChildren:
d.  ng g m z-module --routing
e.  ng g m z-module/catalogs --routing --route catalogs --module=z-module
f. change app.route.tenant.ts line 32, make it point to ZModuleModule
g.  npm run gen-app-routes
h. verify an error is thrown

Test the new script works when duplicate modules are found

a. Perform steps a. through f. from the previous test case
b. Place the fixed route-analyzer under the node_modules folder
c. npm run gen-app-routes
d. verify no error is thrown

Does this PR introduce a breaking change?

  • Yes
  • No

absolutePath = path.join(programDirectory, moduleFolderPath, loadChildrenImportPath);
}

// '.ts' is often omitted on TS imports.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the ts extension disallowed in imports? The compiler is often looking for .d.ts files or it could even be a JS file? Can you explain in a comment when .ts is present?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got it mixed up with js. You are correct, this never happens... And never will happen. Found this read to be interesting: microsoft/TypeScript#35589

@kepelrs kepelrs force-pushed the topic/dbarreto/route-analyzer-unique-loadchildren branch from 44d8eb6 to fd4fd1a Compare January 18, 2023 09:05

// Using absolute path prevents issues when two module files have the exact same name
function getLoadChildrenAbsolutePath(moduleFolderPath: string, loadChildrenImportPath: string): string {
let absolutePath: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpicking since const is preferred

const absolutePath = path.isAbsolute(moduleFolderPath 
   ?  path.join(moduleFolderPath, loadChildrenImportPath) 
   :  path.join(programDirectory, moduleFolderPath, loadChildrenImportPath);

// Must append the file extension, as the TS imports never include it
return `${absolutePath}.ts`;

@@ -6,6 +6,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [Unreleased]
- Fix route-analyzer to use absolute paths instead of basenames
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the header and include something that helps the user know what happened instead of a description of the code change.

### [Fixed]
Fixed bug that broke the route-analyzer when two routing modules had the same base name

### Fixed

@kepelrs kepelrs force-pushed the topic/dbarreto/route-analyzer-unique-loadchildren branch from fd4fd1a to 26403a9 Compare January 18, 2023 14:03
Before this commit, the route-analyzer would search for modules by their basenames (my-module.ts). This could match the wrong module in cases where we had two module files with same name but in different folders.

This change fixes the issue by matching absolute paths instead of only basenames.

Signed-off-by: Davi Barreto <[email protected]>
@kepelrs kepelrs force-pushed the topic/dbarreto/route-analyzer-unique-loadchildren branch from 26403a9 to be171b0 Compare January 18, 2023 14:05
@juanmendes juanmendes merged commit 15fd4cb into vmware-archive:master Jan 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants