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

Change how spfx doctor detects SPFx version #4213

Closed
waldekmastykarz opened this issue Dec 9, 2022 · 14 comments
Closed

Change how spfx doctor detects SPFx version #4213

waldekmastykarz opened this issue Dec 9, 2022 · 14 comments

Comments

@waldekmastykarz
Copy link
Member

Currently, spfx doctor detects SPFx version using npm which looks into node_modules installed in the working director. If npm doesn't return any results, spfx doctor assumes there's no SPFx project and looks for the global SPFx version instead. This can lead to confusing results when you run spfx doctor in an SPFx project that doesn't have dependencies restored yet. We should reconsider how we detect local dependencies and more clearly communicate what we're doing to avoid confusing users.

@Jwaegebaert
Copy link
Contributor

Sounds like a solid idea 👍

We could reference the package.json file or .yo-rc.json when we don't get any result from node_modules.

@milanholemans
Copy link
Contributor

Good idea indeed, I assume we should check the .yo-rc.json like we do with other SPFx commands?

@waldekmastykarz
Copy link
Member Author

We've been using .yo-rc.json in other places, because in the past there were versions of SPFx where not all packages had the same version. In this case, we started with a different approach, because we check two different things: first we check the version in the project and then in the environment, but I think it would be more reliable, to check the version of the project in the same way we do in other places, and then if we see that we're not in a project, defer to the global check like we're doing already.

@Adam-it
Copy link
Contributor

Adam-it commented Jan 7, 2023

sounds like a plan 👍

@waldekmastykarz waldekmastykarz self-assigned this Jan 9, 2023
@waldekmastykarz
Copy link
Member Author

Proposed solution:

  • replace the current project version detection, by reading the SPFx version from .yo-rc.json like we do in spfx project upgrade
  • if .yo-rc.json doesn't exist or it doesn't contain the version number, use the current global SPFx version detection mechanism

Any feedback?

@milanholemans
Copy link
Contributor

Any feedback?

Nope! Seems like a solid plan.

@Adam-it
Copy link
Contributor

Adam-it commented Jan 12, 2023

Any feedback?

Nope! Seems like a solid plan.

+1 👍

@MathijsVerbeeck
Copy link
Contributor

I could work on this!

@milanholemans
Copy link
Contributor

Nice! All yours!

@MathijsVerbeeck
Copy link
Contributor

@waldekmastykarz Just a minor question, am I correct to assumte that this should only be implemented at the method getSharePointFrameworkVersion? And am I allowed to change the extends of the class to BaseProjectCommand?

@waldekmastykarz
Copy link
Member Author

@MathijsVerbeeck correct, the logic is in the getSharePointFrameworkVersion method. What would you like to change about the BaseProjectCommand class?

@MathijsVerbeeck
Copy link
Contributor

MathijsVerbeeck commented Feb 9, 2023

I would change nothing about the BaseProjectCommand class, but currenty, the spfx doctor commands inherits from AnonymousCommand, and I would like to reuse the method from BaseProjectCommand 😄

@waldekmastykarz
Copy link
Member Author

Got it. That makes sense. While you don't need the project loading logic, you'll be able to benefit from project root- and version detection.

@MathijsVerbeeck
Copy link
Contributor

Got it. That makes sense. While you don't need the project loading logic, you'll be able to benefit from project root- and version detection.

Exactly, that's what I would use it for indeed. Thanks for the feedback!

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

Successfully merging a pull request may close this issue.

5 participants