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

Proposal for further front-end cleanups #1827

Open
adamzap opened this issue Dec 15, 2024 · 12 comments
Open

Proposal for further front-end cleanups #1827

adamzap opened this issue Dec 15, 2024 · 12 comments
Assignees

Comments

@adamzap
Copy link
Member

adamzap commented Dec 15, 2024

I'm curious if the maintainers would accept pull requests for the following front-end changes. Some of these ideas are more opinionated or need more guidance than my previous PRs, so I wanted to get feedback first.

Remove bower (#1830/#1832)

As its website says, bower is maintained but not recommended for use by its developers. Our bower.json file now only includes two dependencies: jquery and jquery-flot.

Instead of using bower, could we instead put jquery.min.js and jquery-flot.min.js directly in djangoproject/static/js/lib/? This would include removing the jquery and jquery-flot directories.

Remove require.js

require.js had a small security release in July 2024, but the release before that was in 2018, and the project is not under active development. I'm still trying to understand the full influence that require.js is having on this project, but removing it would require at least the following changes in addition to removing the library itself:

  1. Remove the setup for require.js in the base.html template
  2. Remove the define calls in our JavaScript modules (example)
  3. Remove or repurpose main.js
    • Option 1: Remove the file and add <script> tags to the templates where needed. Adding a new template block for JavaScript includes would also be a good idea. This approach is simple but is riskier and requires more work and testing, but it's the one I would choose.
    • Option 2: Keep the file for its class-based logic and dynamically add <script> tags to the DOM based on it. This approach requires less changes and may be a good way just to remove require.js without broader changes.

Which approach would be preferred here, or is there a better way than what I presented above?

Remove Boilerplate in Many JavaScript Modules

Many of our JavaScript modules define an enclosing function then add an init method to its prototype. This is where the actual logic lives. It seems to me that we could decompose these and just run everything in the top-level enclosing function. Perhaps the current pattern is related to require.js?

Could these modules be simplified, or is this kind of refactor too unnecessary?

Upgrade or Remove jQuery

We're currently using jQuery v1.11.2, and the latest release is v3.7.1. Jumping ahead through ten years of releases could be a pain, but I think it's worth it if jQuery is valued in this project.

Another option would be to remove jQuery instead. We could convert one module at a time to native browser APIs before its removal. Doing this would probably force a replacement of jquery.flot for the dashboard app.

Integrate a JavaScript Linter/Formatter

I'd be willing to work on this, but I'd want guidance on which tool to use and the settings for it.


Thanks for considering these!

@pauloxnet
Copy link
Member

Thanks, @adamzap, for all your work and effort to modernize the frontend of the website.

All your proposal seems ok for me. I would rather remove jQuery totally than upgrade it, also if this requires more PR.

@bmispelon
Copy link
Member

TLDR: Yes, yes, yes, yes, and yes! 😁

One thing I'd like to mention first is that as far as I'm concerned, "maintainers" include you at this point. In fact if you're OK with that, I'd like to add you to the maintainer group of the repository. It shouldn't change the day-to-day work, but there would be a few extra features like assigning you to tickets/prs, you being able to manage labels, ...

As for your proposal, I'm in favor of all the points, and I've put details under each heading.

Logistically, I think we can use this ticket as a progress tracker of sort while you sort out individual PRs for the different sections. If we find that things are getting hard to keep track of, we can open separate tickets for some subtasks (I'm thinking for example of the jquery removal, which might be a longer running task than the others).

Remove bower

Looking at the history of the file, it seems to me that it was originally intended as a way to install js libraries and also keep them updated. Things haven't really turned out this way, and in the last 8 years the only changes made to the file were the cleanups you've been doing the past few months.

For these reasons I'm very much in favor of dropping it and simplify our js structure 👍

Remove require.js

Out all the changes you propose, I think this is the biggest one, and possibly with the highest risk for breakage. But the fact that the project is not being maintained is definitely a good argument for its removal, so I'm tempted to accept.

Looking briefly at the code, our mod directory contains 16 files that total about 25 kb (of which 3.7kb are used by switch-dark-mode.js which is not a require.js module at all). The require.js file itself is 15kb. To me this indicates that there's a 3rd option that could be viable: put all mods into a single file and include it on all pages. We lose in flexibility and might increase the js payload of the first request, but it makes for straightforward code and should guarantee no breakage (at least none caused by a missig mod). If we want to be a bit more clever about it, we could split into two: stripe vs. non-stripe related mods (since the stripe mods should only be included on the donation page I think).

Remove Boilerplate in Many JavaScript Modules

This is covered by the removal of require.js above I think, and as such should be done as part of the same PR.

Upgrade or Remove jQuery

I think jQuery is valued insofar as that's what was used back when the site was made (and back then it was basically the best option). I don't think there's a particular attachment to it otherwise, especially not if we can use native browser APIs.

Here are the steps I'd suggest:

  1. Upgrade jquery straight to the latest version (and probably jquery-flot at the same time). In my limited experience jquery has a pretty good backwards compatibility, and we don't seem to be using that much of it (git grep -E '\$[.(]' -- '*.js' ':!djangoproject/static/js/lib' yields less than 100 results).
  2. Rewrite our code to progressively get rid of jquery in favor of native API where it makes sense. If a better alternative than jquery-flot exists and fulfills our requirements then we could switch to it, but I think it's also fine to keep it: it seems to be doing is job well enough from the little I've seen.

I'm suggesting to start by upgrading jquery first based on the assumptions that 1) it should be quite painless and 2) removing it completely will take some time. If those two assumptions are correct, then it would be nice to have an up-to-date jquery during the transition period.

Integrate a JavaScript Linter/Formatter

I do agree that any serious work on the javascript code should probably start by setting up a linter and autoformatter. I have no strong opinion on this. Back on code.djangoproject.com @ulgens has been working on adding prettier via pre-commit (django/code.djangoproject.com#247), what about doing the same here?


Super excited about all these changes and I'm looking forward to reviewing all the PRs 🎸

@adamzap
Copy link
Member Author

adamzap commented Dec 17, 2024

@bmispelon It's very kind of you to offer me that role. I accept!

Thanks for the feedback on my cleanup suggestions. I will take some time to think about it.

While I was reading your comment, another idea emerged: what if I go through the JavaScript files in the mod directory one at a time (one PR per file) and try to refactor them with all of the above suggestions considered?

  • Remove jQuery
  • Remove code related to require.js
  • Remove init-style structure
  • Make code style consistent
  • Move the inclusion of the file from main.js to a <script> tag in the relevant template(s)

I could try with one of the simpler files first and see if we like the pattern. In my mind, this would clear away the outdated parts so that we can see what kind of complexity we're really dealing with. Testing the changes should be easier too since they will be deep instead of broad.

@pauloxnet
Copy link
Member

... what if I go through the JavaScript files in the mod directory one at a time (one PR per file) and try to refactor them with all of the above suggestions considered?
* Remove jQuery
* Remove code related to require.js
* Remove init-style structure
* Make code style consistent
* Move the inclusion of the file from main.js to a <script> tag in the relevant template(s)
I could try with one of the simpler files first and see if we like the pattern. In my mind, this would clear away the outdated parts so that we can see what kind of complexity we're really dealing with. Testing the changes should be easier too since they will be deep instead of broad.

I agree with everything @bmispelon wrote, and I liked your plan.

I would just add that in reviewing the JavaScript files to modernize them, and at the same time remove old libraries or JQuery, I would also try to understand if part of the JavaScript code can be removed because it was initially useful to give dynamism to the pages, a goal that can now be achieved by taking advantage of the support of modern browsers to HTML5 (e.g.: tag details).

@adamzap
Copy link
Member Author

adamzap commented Dec 17, 2024

@pauloxnet Yes, I'm hoping at least some of the JavaScript can be removed altogether!

@adamzap
Copy link
Member Author

adamzap commented Dec 17, 2024

A few questions as I start this process:

  1. Do we want to continue using four spaces for indenting JavaScript, or should we switch to two spaces? I'm used to two spaces, but I defer.
  2. Which of the following styles is preferred for modules? We'll use doc-switcher.js as an example, but I'm hoping I can remove it completely in favor of a details element.

Original File

define([
    'jquery' //requires jquery
], function( $ ) {

    var DocSwitcher = function(switchers) {
        this.switchers = $(switchers);
        this.init();
    };

    DocSwitcher.prototype = {
        init: function(){
            var self = this;
            $(document).ready(function () {
                // Make version switcher clickable for touch devices

                self.switchers.find('li.current').on('click', function () {
                    $(this).closest("ul").toggleClass('open');
                });
            });
        }
    };

    // Export a single instance of our module:
    return new DocSwitcher('.doc-switcher');
});

Rewritten File with Enclosing Scope

We shouldn't need to check that the document is ready/loaded because we'll include all JavaScript modules just before the closing body tag.

(function () {
    document.querySelectorAll('.doc-switcher li.current').forEach(function (el) {
        el.addEventListener('click', function () {
            this.parentElement.classList.toggle('open');
        });
    });
})();

Rewritten File with No Enclosing Scope

There is no global variable leakage, so I don't think we need the enclosing scope.

document.querySelectorAll('.doc-switcher li.current').forEach(function (el) {
    el.addEventListener('click', function () {
        this.parentElement.classList.toggle('open');
    });
});

Rewritten File with No Enclosing Scope with DOM Utilities

We could add a few lines to base.html before the modules' script tags to make the JavaScript code less verbose. I like this approach, but I don't feel strongly about it.

const $ = document.querySelector;  // Defined in `base.html`
const $$ = document.querySelectorAll;  // Defined in `base.html`

EventTarget.prototype.on = EventTarget.prototype.addEventListener;  // Defined in `base.html`


$$('.doc-switcher li.current').forEach(function (el) {
    el.on('click', function () {
        this.parentElement.classList.toggle('open');
    });
});

@adamzap adamzap self-assigned this Dec 17, 2024
bmispelon pushed a commit that referenced this issue Dec 17, 2024
`bower` is maintained but not recommended for use by its developers. Our
`bower.json` file now only includes two dependencies: `jquery` and
`jquery-flot`. At this point, `bower` is not doing much to serve the
project.

Refs #1827
bmispelon pushed a commit that referenced this issue Dec 17, 2024
`bower` is maintained but not recommended for use by its developers. Our
`bower.json` file now only includes two dependencies: `jquery` and
`jquery-flot`. At this point, `bower` is not doing much to serve the
project.

This is a redo of 98e20a0 which
was reverted because it led to errors in production when running
collectstatic.

Refs #1827
@adamzap
Copy link
Member Author

adamzap commented Dec 18, 2024

@bmispelon The more I look at the code, the more I like your idea of having only one JavaScript file that's included on every page. Good insight there! It's just not very much code. I can do the Stripe parts last to see if they are worth putting in a separate file.

@bmispelon
Copy link
Member

  1. Do we want to continue using four spaces for indenting JavaScript, or should we switch to two spaces? I'm used to two spaces, but I defer.

I don't have a preference, as long as it's enforced by an autoformatter and checked in CI. I don't think the js code has been actively changed in years, so at this point I think you get to decide what you like better 😁

2. Which of the following styles is preferred for modules? We'll use `doc-switcher.js` as an example, but I'm hoping I can remove it completely in favor of a `details` element.

My personal order of preference (from most to least preferred) would be: 2 (unless there's things that might leak out of scope), 1 then 3 (the dollar-based names seem a big gimmicky to me and I like plain js). But again I don't have a very strong preference, so go with what feels right to you.

@pauloxnet
Copy link
Member

  1. ... for indenting JavaScript ... should we switch to two spaces?

I don't have a preference, as long as it's enforced by an autoformatter and checked in CI ... I think you get to decide what you like better 😁

I totally agree

  1. Which of the following styles is preferred for modules?

My personal order of preference (from most to least preferred) would be: 2 ... But again I don't have a very strong preference, so go with what feels right to you.

Same for me

@adamzap
Copy link
Member Author

adamzap commented Dec 18, 2024

Thanks for the feedback. I'll go with two spaces and the second style. I won't be offended that no one likes my three-line JavaScript framework! 😉

@thibaudcolas
Copy link
Member

I’d rather stay out of any decision, but would recommend considering ES modules with dynamic import() as the simplest refactoring when moving away from require.js. It also supports conditional loading, so would largely be a matter of removing the require.js define boilerplate. I suspect keeping different scripts separate would help with long-term maintainability.

@adamzap
Copy link
Member Author

adamzap commented Dec 18, 2024

@thibaudcolas Interesting! I don't have any experience with that approach, but I can start looking into it if that would be the collective desire. The approach I started with in #1835 that was suggested above seems simple to me, but I am biased toward "classic" JavaScript.

bmispelon pushed a commit that referenced this issue Dec 20, 2024
This patch removes the global `djangoproject/static/*` exclude rule in
`.pre-commit-config.yaml` to enable `prettier` to run on this project's
JavaScript files. The `djangoproject/static/js/lib/` directory is now
excluded in the `prettier` hook because there is no value in
reformatting a minified JavaScript file.

I also added a `.prettierrc` file to the root of the project to control
JavaScript formatting.

Refs #1827
bmispelon pushed a commit that referenced this issue Dec 20, 2024
- Simplified code
- Stopped using jQuery
- Moved refactored code to `app.js`

This is the first in a series of patches that will move JavaScript code
out of `require.js` modules and into a single file while also
refactoring.

This patch should bring no user-facing changes.

Refs #1827
adamzap added a commit to adamzap/djangoproject.com that referenced this issue Dec 21, 2024
- Simplified code
- Stopped using jQuery
- Moved refactored code to `djangoproject.js`

This patch should bring no user-facing changes.

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

No branches or pull requests

4 participants