-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Added esupgrade pre-commit hook #2421
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
base: main
Are you sure you want to change the base?
Conversation
codingjoe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, that was interesting. I still found some edge cases that need handling. And I'll work on class transformation.
|
Ok, there is some name shadowing happening in one file that threw me off. I reviewed all changes again, and they all appear correct. If anyone thinks this is helpful, go ahead. Otherwise, just close the PR. I am happy to have tested such a history-rich code base. 😉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request adds the esupgrade pre-commit hook to automatically modernize JavaScript code, converting ES5 syntax to modern ES6+ patterns. The changes include converting var to const/let, traditional functions to arrow functions, string concatenation to template literals, and forEach loops to for...of loops.
- Adds esupgrade pre-commit hook configuration with exclusion for library files
- Modernizes JavaScript across 11 files with ES6+ syntax updates
- Updates variable declarations, function expressions, and string interpolation patterns
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
.pre-commit-config.yaml |
Adds esupgrade hook at version 2025.3.4 with library exclusions |
djangoproject/static/js/mod/switch-dark-mode.js |
Converts var to let, arrow functions, and template literals |
djangoproject/static/js/mod/stripe-donation.js |
Updates to const/let, arrow functions, and modern syntax |
djangoproject/static/js/mod/stripe-change-card.js |
Converts to ES6+ with const declarations and arrow functions |
djangoproject/static/js/mod/list-collapsing.js |
Modernizes with const, arrow functions, and cleaner syntax |
djangoproject/static/js/main.js |
Updates function and variable declarations to ES6+ |
djangoproject/static/js/fundraising-heart.js |
Converts IIFE and functions to arrow syntax with const/let |
djangoproject/static/js/djangoproject.js |
Replaces forEach with for...of loops and arrow functions |
djangoproject/static/js/dashboard/utils.js |
Updates to const/let and template literals |
djangoproject/static/js/dashboard/index.js |
Modernizes with const and arrow functions |
djangoproject/static/js/dashboard/detail.js |
Converts to ES6+ patterns with proper scoping |
djangoproject/static/js/admin_blog_imageupload.js |
Updates to for...of loops and arrow functions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Those co-pilot comments are all legit too. |
Thanks for the PR I'll ask @adamzap and other @django/django-website to review it and share their feedback |
|
OK, let me address the co-pilot changes too in a separate commit. |
|
Not a blocker, but I'd recommend limiting this PR to hook change and linter-related safe (automatic) / unsafe (manual) changes. Copilot is doing a poor job of understanding the scope of a PR and it makes many unrelated comments. Also, I'll recommend installing the hooks locally via Apart from these two points, LGTM. |
|
I tried keeping all commits neatly separate. I can split the manual changes into a separate PR if that makes reviewing them easier. |
I don't think they make an important difference for the review; it would be a small improvement for the git history. (It was a nitpick 🌻 ) |
We ensure the cookie string is a uniform `<space>;` sequence and split.
|
@ulgens I kept the upgrade commit separate to add it to But I can squash them. Whatever works for you. I upgrade the commit to include the latest changes on main. I also upgrade esupgrade to 2025.8 |
|
@codingjoe Oh okay, that makes sense. We generally see these fix commits from pre-commit.ci when people forget to install hooks locally; I thought it was the case here. I was about to recommend keeping the commit separate for the reason you shared, but creating the commit yourself and explanining the "blame ignore revs" in commit message, but I see that you already squashed. This was a nitpick to start with, I'm okay with both options. |
|
@codingjoe thanks for taking care of this PR Do we have a way to measure performance or feature regression eventually connected with this big PR? I would really like to read an opinion from @adamzap or @sarahboyce about this PR before approve it |
Could you elaborate on that? I don't see how this PR would affect performance. |
Since the PR is quite big, do we have a way compare performance before and after applying the code changes? It would be something interesting to have to prove that a modern Javascript means also better performance or at least to prove that the new code is as fast as before. |
|
I see. I personally don't expect any performance improvements from a lint/styling PR, but I also see your point. Do you consider not having that data as a blocker for this PR? |
|
I think I'd like to go with whatever the WG thinks is best here. I intentionally don't write the most "modern" JavaScript, so it modified some of my code. Regardless, I think a lot of manual testing will need to happen if this is to be merged. Has that been done? Either way, this is good motivation for me to finish removing jQuery! 😉 |
What's your motivation to avoid some modern JavaScript code?
That's a good point. I think we can write a list of things to be tested manually.
This is a good side effect 😀 |
Look what I read early on HN: https://waspdev.com/articles/2026-01-01/javascript-for-of-loops-are-actually-fast
All changes are baseline "widely available" and thus have been supported by all browsers for over 30 months. The Baseline project was established to update with confidence. esupgrade has two modes: this is the "boring" one. 😉 "If you don't touch it, you can't break it." – True, but I already had to rebase twice. So the code is being touched. And the more Pythonic modern syntax will hopefully prevent future errors.
Yes, I have done that to the best of my ability. Yet, I am fairly new to this project, sooo I don't mind a second set of eyes. Lastly, as mentioned in the DSF-member Discord, I am still looking for members to form a JS work group. 🚀 |
Uh oh!
There was an error while loading. Please reload this page.