Skip to content

[heft-lint] Adds support for using lint plugin without typescript phase #5239

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

benjamind
Copy link

Summary

The heft-lint-plugin currently requires a typescript plugin to be running in the same phase to retrieve the list of changed files. This makes it more efficient, but also prevents you from using the lint plugin in other standalone heft tasks.

See zulip thread here : https://rushstack.zulipchat.com/#narrow/channel/262522-heft/topic/.E2.9C.94.20.5Bheft-lint-plugin.5D.20Can.20it.20run.20on.20its.20own.3F/with/440265323

Details

Modified the plugin to create a TSProgram and extract the list of source files for the tsconfig when the typescript plugin is not accessed from the heft task session.

How it was tested

Manually used the plugin within another repository and confirmed it runs. Looks like this plugin has no real tests right now?

Impacted documentation

@benjamind
Copy link
Author

Getting some test failures in unrelated packages? Not sure how they were impacted by this change...

Copy link
Contributor

@dmichon-msft dmichon-msft left a comment

Choose a reason for hiding this comment

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

Nothing may execute inside of apply, the actual work needs to happen in runAsync to ensure correct timing.
The trickery with the TypeScript accessor somewhat hides the execution but ensures it happens after TypeScript has finished.

@benjamind
Copy link
Author

I refactored a fair bit in response to review feedback, ready for another pass I think.

@benjamind
Copy link
Author

@iclanton @dmichon-msft should be ready for a final review now.

Copy link
Contributor

@dmichon-msft dmichon-msft left a comment

Choose a reason for hiding this comment

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

Looks pretty good now, though there are some simplifications that may be worth making.

@benjamind
Copy link
Author

@iclanton @dmichon-msft any chance we could get this merged and published today I'd like to replace the official implementation into my local repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Needs triage
Development

Successfully merging this pull request may close these issues.

3 participants