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

build: migrate to Nx v17 inc deps #252

Closed
wants to merge 17 commits into from
Closed

build: migrate to Nx v17 inc deps #252

wants to merge 17 commits into from

Conversation

BioPhoton
Copy link
Collaborator

@BioPhoton BioPhoton commented Nov 14, 2023

This PR updates Nx packages. In addition to the new package versions a couple of refactoring's needed to be done to make it work.

Included changes:

  • package updates
  • refactoring of the nx-plugin code due to changes of the nx.json structure
  • refactorings of the plugin-eslint package due to dependency changes

Included changes to discuss:

  • the current PR includes a bigger refactoring of the tests in package-eslint
  • the number of rules got reduces
  • test setup per case

@BioPhoton BioPhoton self-assigned this Nov 14, 2023
@BioPhoton
Copy link
Collaborator Author

I don't really get why the eslint-plugin tests fail... @vmasek any clue?

@BioPhoton
Copy link
Collaborator Author

BioPhoton commented Nov 14, 2023

I know now why the tests where failing. They rely on real eslint setup and the ouatput data changed.
@matejchalk I already created #255 and #254 and updated #224

What would be the minimum fix to make this PR turn 🟢?

Update:
For now I went with 8f15475 let me know what you think

Update:
I should not change the eslint setup in the package as it tests the integration of the other plugins

@matejchalk
Copy link
Collaborator

@BioPhoton Why is this a precondition for #126? Are we not able to publish packages until we're on Nx 17? 🤨 That would surprise me, because I was able to publish the @code-pushup/portal-client using @jscutlery/semver and Nx 16 without any issues.

@BioPhoton
Copy link
Collaborator Author

BioPhoton commented Nov 15, 2023

I wanted to get the update in before, but also decided today that its not a good idea.

@BioPhoton
Copy link
Collaborator Author

BioPhoton commented Nov 19, 2023

Ok, I gave the test refactoring a try based on our discussions with @Tlacenka. It's not perfect but much more entangles and smaller in rules.

@matejchalk let me know if I can polish this version or if I can adopt it.

If it is OK I would also try to generate the eslint files dynamically instead of having them in the fs.

@BioPhoton BioPhoton requested a review from matejchalk November 20, 2023 18:32
Copy link
Collaborator

@matejchalk matejchalk left a comment

Choose a reason for hiding this comment

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

Sorry, but the PR scope is too big. Refactoring the plugin-eslint tests isn't necessary to update Nx. Please revert those changes and just run npx nx test plugin-eslint -u to update the snapshot.

The fact that the tests failed after a major update isn't a bad thing, in my opinion. It gives us visibility into the impact of those changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The migrations.json file shouldn't be committed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just learned that it is not needed anymore

nx.json Show resolved Hide resolved
@BioPhoton
Copy link
Collaborator Author

BioPhoton commented Nov 22, 2023

@matejchalk
I will first refactor the plugin-eslint tests and then update Nx.

@matejchalk
Copy link
Collaborator

matejchalk commented Nov 22, 2023

I will first refactor the plugin-eslint tests and then update Nx.

But why? 😅 The priorities are completely the opposite in my opinion. I'm not even convinced the ESLint tests need to be refactored (would be good to get @Tlacenka's opinion on this), and it surely isn't a blocker for updating Nx (npx nx test plugin-eslint -u is the only necessary step, I believe).

I honestly don't like having to decline your PRs, but when you decide to make big changes before first consulting it with us (e.g. on refinement), then I'm afraid it's gonna keep happening 😞

@Tlacenka
Copy link
Collaborator

Tlacenka commented Nov 22, 2023

@BioPhoton Ok, I gave the test refactoring a try based on our discussions with @Tlacenka. It's not perfect but much more entangles and smaller in rules.

@matejchalk I'm not even convinced the ESLint tests need to be refactored (would be good to get @Tlacenka's opinion on this)

The plugin-eslint package in the original version is in my opinion sufficiently covered by both unit (internal logic) and integration (initiation and executing the runner) tests.

I am unfortunately unable to determine which changes are truly necessary due to the updated nx version. The ESLint rules and results seem to be different, variables renamed.

Should the changes be done as a part of #255 , they should in my opinion be first investigated in detail and any targetted changes should be scoped and prioritised separately (otherwise the issue could get bloated easily), not as a part of this PR.

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

Successfully merging this pull request may close these issues.

3 participants