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.

Sign up for GitHub

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

Jump to bottom

cleanup(repo): remove unused dependencies #27676

Merged
merged 9 commits into from
Dec 19, 2024
Merged

cleanup(repo): remove unused dependencies #27676

merged 9 commits into from
Dec 19, 2024

Conversation

Phillip9587
Copy link
Contributor

See individual commits

Current Behavior

Expected Behavior

Related Issue(s)

Fixes #

@Phillip9587 Phillip9587 requested review from a team as code owners August 28, 2024 13:51
@Phillip9587 Phillip9587 requested a review from JamesHenry August 28, 2024 13:51
@vercel Vercel
Copy link

vercel bot commented Aug 28, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
nx-dev ✅ Ready (Inspect) Visit Preview Dec 19, 2024 1:37pm

@nx-cloud Nx Cloud
Copy link

nx-cloud bot commented Aug 28, 2024 •
edited
Loading

View your CI Pipeline Execution ↗ for commit e3322a8.

Command Status Duration Result
nx affected --targets=lint,test,build,e2e,e2e-c... ✅ Succeeded 36m 56s View ↗
nx run-many -t check-imports check-commit check... ✅ Succeeded 58s View ↗
nx-cloud record -- nx-cloud conformance:check ✅ Succeeded 1s View ↗
nx-cloud record -- nx format:check --base=dfd50... ✅ Succeeded 18s View ↗
nx-cloud record -- nx sync:check ✅ Succeeded 17s View ↗
nx documentation --no-dte ✅ Succeeded 1m 6s View ↗

☁️ Nx Cloud last updated this comment at 2024-12-19 14:13:12 UTC

@Phillip9587
Copy link
Contributor Author

Hi @JamesHenry,

I hope you're doing well! I just wanted to kindly follow up on this PR as it has been open for about a month without any reviews. Whenever you have a chance, could you please take a look? Let me know if there’s anything I can do to help move this forward.

Thanks again for your time and consideration!

@JamesHenry JamesHenry self-assigned this Sep 25, 2024
@JamesHenry
Copy link
Collaborator

LGTM, thanks, sorry just needs a conflict fix up in the lock file

@JamesHenry
Copy link
Collaborator

I pushed it up myself

@JamesHenry JamesHenry enabled auto-merge (squash) October 1, 2024 12:30
@JamesHenry
Copy link
Collaborator

JamesHenry commented Oct 1, 2024 •
edited
Loading

@Phillip9587 I'm afraid it looks like webpack-sources is used after all now. I think it's because the rspack plugin was promoted from labs into this repo. Please could you take another look? It's possible that other webpack utilities are relevant

JamesHenry
JamesHenry requested changes Oct 1, 2024
View reviewed changes
Copy link
Collaborator

@JamesHenry JamesHenry left a comment

Choose a reason for hiding this comment

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

See comment

@Phillip9587
Copy link
Contributor Author

Phillip9587 commented Oct 1, 2024 •
edited
Loading

@JamesHenry I updated the rspack plugin code to import webpack sources through the rspack reexport. See https://rspack.dev/api/javascript-api/index#sources-object

@JamesHenry JamesHenry requested a review from Coly010 October 1, 2024 14:35
@Phillip9587 Phillip9587 mentioned this pull request Oct 1, 2024
Merged
@Phillip9587 Phillip9587 mentioned this pull request Oct 15, 2024
Merged
@Phillip9587
Copy link
Contributor Author

I opened #28447 to remove the webpack-sources import to maybe get this merged sooner.

Coly010 added a commit that referenced this pull request Oct 16, 2024
@Phillip9587 @xiongemi @Coly010
fix(rspack): add dependency-checks lint rule (#28225)
4b6c831 
<!-- Please make sure you have read the submission guidelines before
posting an PR -->
<!--
https://github.com/nrwl/nx/blob/master/CONTRIBUTING.md#-submitting-a-pr
-->

<!-- Please make sure that your commit message follows our format -->
<!-- Example: `fix(nx): must begin with lowercase` -->

<!-- If this is a particularly complex change or feature addition, you
can request a dedicated Nx release for this pull request branch. Mention
someone from the Nx team or the `@nrwl/nx-pipelines-reviewers` and they
will confirm if the PR warrants its own release for testing purposes,
and generate it for you if appropriate. -->

## Current Behavior
<!-- This is the behavior we have today -->

The rspack plugin has no dependency-checks in place. I discovered it
while working on #27676.

## Expected Behavior
<!-- This is the behavior we should expect with the changes in this PR
-->

The `@nx/dependency-checks` eslint rule should be used.

There are a few dependencies that I don't know how to proceed with:

```
The "rspack" project uses the following packages, but they are missing from "dependencies":
    - @nx/workspace
    - webpack-sources
    - @module-federation/sdk  @nx/dependency-checks

The "@rspack/plugin-minify" package is not used by "rspack" project
```

## Related Issue(s)
<!-- Please link the issue being fixed so it gets closed when this is
merged. -->

#27676

---------

Co-authored-by: Emily Xiong <xiongemi@gmail.com>
Co-authored-by: Colum Ferry <cferry09@gmail.com>
jaysoo pushed a commit that referenced this pull request Oct 17, 2024
@Phillip9587 @xiongemi
@Coly010 @jaysoo
fix(rspack): add dependency-checks lint rule (#28225)
6cb2af4 
<!-- Please make sure you have read the submission guidelines before
posting an PR -->
<!--
https://github.com/nrwl/nx/blob/master/CONTRIBUTING.md#-submitting-a-pr
-->

<!-- Please make sure that your commit message follows our format -->
<!-- Example: `fix(nx): must begin with lowercase` -->

<!-- If this is a particularly complex change or feature addition, you
can request a dedicated Nx release for this pull request branch. Mention
someone from the Nx team or the `@nrwl/nx-pipelines-reviewers` and they
will confirm if the PR warrants its own release for testing purposes,
and generate it for you if appropriate. -->

## Current Behavior
<!-- This is the behavior we have today -->

The rspack plugin has no dependency-checks in place. I discovered it
while working on #27676.

## Expected Behavior
<!-- This is the behavior we should expect with the changes in this PR
-->

The `@nx/dependency-checks` eslint rule should be used.

There are a few dependencies that I don't know how to proceed with:

```
The "rspack" project uses the following packages, but they are missing from "dependencies":
    - @nx/workspace
    - webpack-sources
    - @module-federation/sdk  @nx/dependency-checks

The "@rspack/plugin-minify" package is not used by "rspack" project
```

## Related Issue(s)
<!-- Please link the issue being fixed so it gets closed when this is
merged. -->

#27676

---------

Co-authored-by: Emily Xiong <xiongemi@gmail.com>
Co-authored-by: Colum Ferry <cferry09@gmail.com>
Coly010 pushed a commit that referenced this pull request Oct 21, 2024
@Phillip9587
fix(rspack): removed webpack-sources import (#28447)
25e3bdd 
removed the webpack-sources import and replaced it with the sources
export from rspack

<!-- Please make sure you have read the submission guidelines before
posting an PR -->
<!--
https://github.com/nrwl/nx/blob/master/CONTRIBUTING.md#-submitting-a-pr
-->

<!-- Please make sure that your commit message follows our format -->
<!-- Example: `fix(nx): must begin with lowercase` -->

<!-- If this is a particularly complex change or feature addition, you
can request a dedicated Nx release for this pull request branch. Mention
someone from the Nx team or the `@nrwl/nx-pipelines-reviewers` and they
will confirm if the PR warrants its own release for testing purposes,
and generate it for you if appropriate. -->

## Current Behavior
<!-- This is the behavior we have today -->
`@nx/rspack` uses `webpack-sources` which is not listed as dependency.
The issue originated in PR #27676 but i decided to split it out. I also
opened #28225 to add the dependency-check rule for rspack.

## Expected Behavior
<!-- This is the behavior we should expect with the changes in this PR
-->
`@nx/rspack` uses the reexport from `@rspack/core` 

https://rspack.dev/api/javascript-api/#sources-object

## Related Issue(s)
<!-- Please link the issue being fixed so it gets closed when this is
merged. -->

#28225 #27676
jaysoo pushed a commit that referenced this pull request Oct 23, 2024
@Phillip9587 @jaysoo
fix(rspack): removed webpack-sources import (#28447)
7f69f27 
removed the webpack-sources import and replaced it with the sources
export from rspack

<!-- Please make sure you have read the submission guidelines before
posting an PR -->
<!--
https://github.com/nrwl/nx/blob/master/CONTRIBUTING.md#-submitting-a-pr
-->

<!-- Please make sure that your commit message follows our format -->
<!-- Example: `fix(nx): must begin with lowercase` -->

<!-- If this is a particularly complex change or feature addition, you
can request a dedicated Nx release for this pull request branch. Mention
someone from the Nx team or the `@nrwl/nx-pipelines-reviewers` and they
will confirm if the PR warrants its own release for testing purposes,
and generate it for you if appropriate. -->

## Current Behavior
<!-- This is the behavior we have today -->
`@nx/rspack` uses `webpack-sources` which is not listed as dependency.
The issue originated in PR #27676 but i decided to split it out. I also
opened #28225 to add the dependency-check rule for rspack.

## Expected Behavior
<!-- This is the behavior we should expect with the changes in this PR
-->
`@nx/rspack` uses the reexport from `@rspack/core` 

https://rspack.dev/api/javascript-api/#sources-object

## Related Issue(s)
<!-- Please link the issue being fixed so it gets closed when this is
merged. -->

#28225 #27676
@JamesHenry
Copy link
Collaborator

@Phillip9587 is this one still relevant? Does it just need a rebase?

@Phillip9587
Copy link
Contributor Author

@JamesHenry Yes it is. I'm going to rebase it. Should i open a PR for each individual removal? Would it be easier to merge multiple PRs?

@Phillip9587
Copy link
Contributor Author

@JamesHenry Can we merge this now? Rebasing takes quite some time and the dependencies are unused so there are no source code changes

@JamesHenry JamesHenry merged commit 7a583da into nrwl:master Dec 19, 2024
6 checks passed
@JamesHenry
Copy link
Collaborator

Thanks again!

@Phillip9587 Phillip9587 deleted the remove-unused-deps branch December 19, 2024 14:50
ndcunningham pushed a commit that referenced this pull request Dec 20, 2024
@Phillip9587 @ndcunningham
cleanup(repo): remove unused dependencies (#27676)
e7104c0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JamesHenry JamesHenry JamesHenry approved these changes

@FrozenPandaz FrozenPandaz Awaiting requested review from FrozenPandaz

@vsavkin vsavkin Awaiting requested review from vsavkin

@AgentEnder AgentEnder Awaiting requested review from AgentEnder

@jaysoo jaysoo Awaiting requested review from jaysoo

@Coly010 Coly010 Awaiting requested review from Coly010

Assignees

@JamesHenry JamesHenry

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Phillip9587 @JamesHenry