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

feat(envs API) - Support custom dep detectors for envs #7199

Merged
merged 20 commits into from
Mar 28, 2023

Conversation

Jinjiang
Copy link
Member

@Jinjiang Jinjiang commented Mar 27, 2023

Proposed Changes

  • Expose detectors() to the new env interface.
  • Revamp the implementation of the precinct and detector hook.
  • Extend the interface DependencyDetector by adding an optional type member, which is better to be further processed rather than ext member.

Something needs to pay attention to:

  1. I didn't find any places to consume precinct.ast. Here I just passed the relevant test cases.
  2. I didn't find any external places to call precinct() except in precinct.paperwork(). Here I just passed the relevant test cases. And since those 2 functions are not nested anymore, I skipped the case: paperwork -> when given detective configuration -> still does not filter out core module by default.

@Jinjiang Jinjiang marked this pull request as draft March 27, 2023 03:56
@Jinjiang Jinjiang marked this pull request as ready for review March 27, 2023 08:44
@davidfirst
Copy link
Member

@Jinjiang , Great job with this PR! :)

Regarding the points you mentioned:

  1. It's populated in precinct index.ts file ast = walker.parse(content); and it's consumed in filling-cabinet/index.ts file: const ast = options.ast || options.content; to determine the js type (amd/commonjs/es6).
    Not sure how important it is, because seems like this is for optimization only to not re-parse. If the ast is not there, it uses the content and then later on parses it. Also, we probably currently treat all JS as ES6, so maybe we can even remove more unneeded stuff there later on.

  2. Makes sense. Yes, I believe it gets called only from paperwork and it's fine to skip that test (or even delete it).

davidfirst
davidfirst previously approved these changes Mar 27, 2023
scopes/dependencies/dependency-resolver/dependency-env.ts Outdated Show resolved Hide resolved
* determine what type of content the detector is for.
* if no type provided, the type would be the ext.
*/
type?: string;
Copy link
Member

Choose a reason for hiding this comment

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

What possible values are for this?
if it's general string, we should improve the comment above it to explain what it is

Copy link
Member Author

Choose a reason for hiding this comment

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

This type is what precinct() previously receives as the second argument. Also, it's what the previous getType() returns in paperwork(). I added this field into the detector interface to combine the 2 steps directly.

The previous implementation has 2 steps: 1) get the type from the ext, 2) get detective from the type.

Sometimes ext names and types are not 1:1 matched. For example, .ts and .tsx both match the type ts.

For custom env detectors, I think it's also a good practice for detector authors to specify what type of files it belongs to.

And I updated the code comment a little bit. What do you think now? :-)

@GiladShoham GiladShoham changed the title Support custom dep detectors for envs feat(envs API) - Support custom dep detectors for envs Mar 28, 2023
@GiladShoham GiladShoham merged commit 82007c3 into master Mar 28, 2023
@GiladShoham GiladShoham deleted the jinjiang/dep-detectors branch March 28, 2023 13:03
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.

3 participants