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

style: remove eslint with frontend code removal ADR #36383

Merged
merged 1 commit into from
Mar 20, 2025

Conversation

robrap
Copy link
Contributor

@robrap robrap commented Mar 14, 2025

Description

  • Add ADR for frontend code removal
  • Drop eslint, as explained in the ADR

Supporting information

See new ADR for details.

Other information

  • package-lock.json has not yet been rebuilt. How is this done properly?
  • eslint related comments have been left in place, and will go away with the javascript files.
  • there may be other clean-up possible, but this is a start.

@robrap robrap force-pushed the robrap/remove-eslint branch from 32141b8 to 2103a9e Compare March 14, 2025 06:55
@bradenmacdonald
Copy link
Contributor

bradenmacdonald commented Mar 14, 2025

package-lock.json has not yet been rebuilt. How is this done properly?

Delete it and run npm install to re-generate it. This may lead to different versions of indirect dependencies being installed though.

Copy link
Contributor

@bradenmacdonald bradenmacdonald left a comment

Choose a reason for hiding this comment

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

This makes sense to me. I haven't tested this branch but I imagine removing lint tooling is low risk.

@robrap robrap force-pushed the robrap/remove-eslint branch from 28c6a49 to e807c7e Compare March 18, 2025 17:28
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Author's Note:

  • This change (updating package-lock.json) broke the build with failing JS tests. :)
  • In devstack, I have npm 10.5.2, and the CI checks are using 10.7.x. I don't know the importance of that for running npm install.
  • The resultant file here seems to still install eslint.
  • There are so many changes, including odd changes like dropping the license from many dependencies, so I'm not sure if I'm just building the file incorrectly.

Possible next steps:

  1. Someone with more expertise and interest decides to land this, or
  2. I open an issue and point to this PR, which will be closed as not-completed. This was meant to be a quick win, worth skipping an issue for, but that has turned out not to be the case for me.
    FYI: @feanil @arbrandes @bradenmacdonald

@kdmccormick
Copy link
Member

I think I'm missing the context that led to this decision. Have people been finding themselves blocked by eslint? I don't feel strongly that we need to keep eslint--just looking to understand the motivation and priority level behind getting rid of it.

@robrap
Copy link
Contributor Author

robrap commented Mar 18, 2025

I think I'm missing the context that led to this decision.

This is partly captured in the ADR in this PR. Some additional context;

  • Gitgub tries to be helpful and creates comments with violations on to PRs, including for files not changed (I think), which incorrectly gives the impression we want these violations fixed.
    • This could be added to the ADR.
  • This came up because @bradenmacdonald was nice enough to start fixing these because they were showing up on PRs, and I/we wanted to save him time for other activities.

@kdmccormick
Copy link
Member

Got it, ty.

Sounds like this not critical but would be a nice-to-have if someone were able to get ot over the line.

@kdmccormick kdmccormick marked this pull request as draft March 18, 2025 19:29
@kdmccormick kdmccormick added maintenance Routine upkeep necessary for the health of the platform help wanted Ready to be picked up by anyone in the community labels Mar 18, 2025
@robrap
Copy link
Contributor Author

robrap commented Mar 18, 2025

Sounds like this not critical but would be a nice-to-have if someone were able to get It over the line.

Exactly. Also, I updated the ADR to provide more details about why this would be nice-to-have.

@robrap robrap self-assigned this Mar 18, 2025
@robrap
Copy link
Contributor Author

robrap commented Mar 18, 2025

@feanil @arbrandes: Let me know before the end of the week (March 21) if you want to take this over, or if you know you don't.

Assuming no one wants to land this now, I will:

  • Create an issue around this that points to this PR.
  • Create a separate PR with just the ADR.
    • Note: This ADR also captures the decision to move away from frontend code in edx-platform.
    • I'll update the ADR to point to the issue for the ESLint removal.

UPDATE: The deadline posted here was simply so I knew whether I needed to take action or not. After discussion, I'm just going to let this sit because we hope someone might land this.

Sorry, something went wrong.

@feanil
Copy link
Contributor

feanil commented Mar 20, 2025

@brian-smith-tcril can you take a look at this and help us figure out how to fix forward?

@feanil feanil assigned feanil and unassigned robrap Mar 20, 2025
@brian-smith-tcril
Copy link
Contributor

Removing eslint makes sense. How was the package-lock.json updated? For sweeping changes I generally just delete it and run npm install to generate a new one.

@feanil
Copy link
Contributor

feanil commented Mar 20, 2025

I've re-bulit the package-lock.json from scratch let's see how that goes. @robrap to close the look, I'll take over the PR and get it landed, thanks for getting it started.

@feanil feanil marked this pull request as ready for review March 20, 2025 14:30
@robrap
Copy link
Contributor Author

robrap commented Mar 20, 2025

@brian-smith-tcril @feanil: I created the lock file the way you described, but maybe I have an incompatible version of node? I don't know and needed to move on. Thank you both.

@feanil feanil force-pushed the robrap/remove-eslint branch from 839fec8 to fbb0a96 Compare March 20, 2025 14:32
@robrap
Copy link
Contributor Author

robrap commented Mar 20, 2025

@feanil: I did not get to the step of building static assets because the tests were failing. Would you be able to build a sandbox?

@feanil feanil force-pushed the robrap/remove-eslint branch from fbb0a96 to b0eff0f Compare March 20, 2025 14:33
@feanil feanil added create-sandbox open-craft-grove should create a sandbox environment from this PR and removed help wanted Ready to be picked up by anyone in the community labels Mar 20, 2025
@feanil
Copy link
Contributor

feanil commented Mar 20, 2025

Got everything squashed and rebased for merging and I've tagged it to create a sandbox now that the static asset issue has been resolved.

- Add ADR for frontend code removal
- Drop eslint, as explained in the ADR
@feanil feanil force-pushed the robrap/remove-eslint branch from b0eff0f to 399be67 Compare March 20, 2025 15:03
@brian-smith-tcril
Copy link
Contributor

@robrap

maybe I have an incompatible version of node?

Ah, yeah, that can definitely cause issues. I didn't mention that I run nvm use first.

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@feanil
Copy link
Contributor

feanil commented Mar 20, 2025

Sandbox looks good, I'm merging this.

@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@robrap
Copy link
Contributor Author

robrap commented Mar 20, 2025

@feanil: Before merging, can you determine why the sandbox failed?

@feanil
Copy link
Contributor

feanil commented Mar 20, 2025

This second sandbox build failed in a apt update unrelated to this change, the first one succeeded so I'm comfortable merging this.

@feanil feanil merged commit 46503e4 into master Mar 20, 2025
49 checks passed
@feanil feanil deleted the robrap/remove-eslint branch March 20, 2025 17:00
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
create-sandbox open-craft-grove should create a sandbox environment from this PR maintenance Routine upkeep necessary for the health of the platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants