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

Implement function to detect facade modules; Adjust depth check in dependency traversal. #114

Merged
merged 8 commits into from
Jul 8, 2020
Merged

Implement function to detect facade modules; Adjust depth check in dependency traversal. #114

merged 8 commits into from
Jul 8, 2020

Conversation

FelixSchuSi
Copy link
Contributor

This PR consists of two small changes:

  1. Fix of the dependency depth check.
    Currently Dependencies are always traversed one level too few.
    E. g. when MAX_EXTERNAL_DEPTH or MAX_INTERNAL_DEPTH are set to 1 not even directly imported modules (depth=1) are evaluated.
  2. Added funtion to detect facade modules.
    Currently external modules are evaluated two modules deep in order to support facade modules.
    Facade modules are modules that only consist of import and export declarations. These types of modules are frequently used in component libraries.
    With this change facade module are detected in dependency traversal and do not increase depth.
    Since facade modules dont increase depth anymore, MAX_EXTERNAL_DEPTH can be reduced to one.
    I have manually tested this with weightless components and carbon-custom-elements . It might be useful to take a look at other component libraries that use facade modules to see if the implemented function detects them corretcly.

@FelixSchuSi
Copy link
Contributor Author

This is weird.
The last commit contains the same changes as the first commit (except for a space), however the tests failed only for the first commit.

I couldnt find out why the tests failed, because all tests pass on my machine an the pipelines logs show an opaque node error.

@runem
Copy link
Owner

runem commented Jul 8, 2020

First of all, thank you so much for this PR! I look forwarding taking a closer look at the code later today when I get some more time. I expect to merge into 1.2.0 shortly thereafter :-)

It would be nice if you add some tests to test/parser/parse-dependencies.ts, but I can also add some tests afterwards 👍

Unfortunately, currently package-locks are not used when installing package dependencies through Lerna, so the CI could be using minor/patch versions with potential bugs. It seems like a bug was introduced in ava yesterday that was fixed within a couple of hours with version 3.10.1.

@runem
Copy link
Owner

runem commented Jul 8, 2020

I just added two commits on top of your work :-)

(1) I added some more tests to parse-dependencies.ts.

(2) I would prefer the parameters "SourceFile" and "TsModule" to be camel cased and for "TsModule" to be named "ts" instead to follow project conventions, so I changed the names of the parameters.

https://github.com/FelixSchuSi/lit-analyzer/blob/ada458e82383038b7fdc1cc83b5b7ef53d4d994d/packages/lit-analyzer/src/analyze/parse/parse-dependencies/visit-dependencies.ts#L153-L165

@runem runem merged commit 3dad31d into runem:1.2.0 Jul 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants