openlibrary icon indicating copy to clipboard operation
openlibrary copied to clipboard

EPIC: Remove inline JS across all python templates

Open jdlrobson opened this issue 5 years ago • 32 comments

NOTE: This will enable OpenLibrary's JavaScript to be async and non-render blocking. When transitioning to async JavaScript it's vitally important we review the DOMContentLoaded event inside ol.analytics to make sure it is still firing.

Various templates across openlibrary add inline JavaScript files making it really hard to make changes to HTML or JS that is built in webpack without the potential to break things

The epic can be considered done when there are no matches for the following query:

# using [silver-searcher](https://github.com/ggreer/the_silver_searcher)
ag "(\<script type=\"text\/ja|\<script\>)"  --file-search-regex .html openlibrary/(macros|templates) -l --ignore "openlibrary/templates/site/body.html" --ignore openlibrary/templates/site/footer.html

This will allow us to drop the window.q handling code and begin to refactor/rewrite our JavaScript to a more modern framework.

The How

The goal is to move JS out of HTML template files into JS files referenced inside openlibrary/plugins/openlibrary/js/index.js

Typically inline scripts are used when we need to pass data from the server to the client. Historically we've dynamically generated JavaScript

There are several ways to use this

  • Use the JS equivalent function
  • If a string use a data attribute, and read the value from the DOM in JS.
  • If an object use a script tag with type="text/json+graph" following the example in openlibrary/templates/publishers/view.html calling json_encode

Recommended steps

  • Understand how the code works and where the code works before starting. Note some templates may be used on different pages.
  • Remove the inline script in the HTML template and move to a file inside openlibrary/plugins/openlibrary/js/.js
  • Load .js inside index.js
  • Run make js
  • Verify the page still works as before without any JS in the error console.
  • Make sure the JS is loaded. See note below.
  • Replace any references to window.q.push with $(fn)
  • Document the new code explaining to your best understanding what it does and make sure it passes eslint
  • If possible, make a video showing before/after of the code working (something like loom makes it easy)

Note: To avoid large amounts of JS in index.js we will import JS as required based on checks on the HTML. For example

if (document.getElementById('modal-link')) {
        import(/* webpackChunkName: "patron_metadata" */ './patron-metadata')
            .then((module) => module.initPatronMetadata());
    }

Note 2: Some JS is duplicated in other files. If that's the case, the goal should be to simplify the existing code to avoid the duplication.

Acceptance criteria

Pick a file or folder in the list below

  • [ ] #9176
  • [x] openlibrary/templates/account/loans.html (medium) #8378
  • [ ] openlibrary/templates/swagger/swaggerui.html (medium) #8381
  • [x] openlibrary/templates/account/ia_thirdparty_logins.html (easy) #8379
  • [x] openlibrary/templates/type/i18n/edit.html #8336
  • [x] openlibrary/templates/type/i18n/view.html #8336
  • [x] openlibrary/templates/lib/edit_head.html (very easy) #8383
  • [x] openlibrary/templates/books/breadcrumb_select.html (easy) #8380
  • [x] openlibrary/templates/account/readinglog_stats.html (medium) (use data-config on div.readinglog-charts element instead) #8382
  • [x] openlibrary/macros/TypeChanger.html #8374
  • [x] openlibrary/macros/LoanStatus.html @jimchamp #8275
  • [x] openlibrary/templates/lists/lists.html #2468
  • [x] openlibrary/templates/lists/widget.html (#6159)
  • [x] openlibrary/templates/merge/authors.html
  • [x] openlibrary/templates/covers/change.html
  • [x] openlibrary/templates/covers/saved.html
  • [x] openlibrary/templates/covers/manage.html @drakene
  • [x] openlibrary/templates/covers/add.html
  • [x] openlibrary/templates/admin/case.html @RayBB #8260
  • [x] openlibrary/templates/subjects.html @Yashs911
  • [x] openlibrary/templates/admin/people/edits.html @drakene
  • [x] openlibrary/templates/admin/people/view.html @drakene
  • [x] openlibrary/templates/books/check.html @nileshtrivedi @CliftonMcCallum
  • [x] openlibrary/templates/books/author-autocomplete.html @RayBB
  • [x] openlibrary/templates/books/edit/addfield.html @RayBB #8261 (previously @cdrini #2700)
  • [x] openlibrary/templates/books/edit/edition.html @RayBB
  • [x] openlibrary/templates/books/edit.html @lephemere
  • [x] openlibrary/templates/books/edit/web.html @lephemere
  • [x] openlibrary/templates/books/edit/excerpts.html
  • [x] openlibrary/templates/history.html
  • [x] openlibrary/templates/type/author/view.html @Yashs911 #5262
  • [x] openlibrary/templates/type/user/edit.html @RayBB
  • [x] openlibrary/templates/type/user/view.html @RayBB
  • [x] openlibrary/templates/type/list/view_body.html #2468
  • [x] openlibrary/templates/work_search.html
  • [x] openlibrary/templates/lib/history.html
  • [x] openlibrary/templates/site/stats.html @Yashs911
  • [x] openlibrary/templates/publishers/view.html
  • [x] openlibrary/templates/account/borrow.html #5644
  • [ ] Write a script to run in CI on PRs to prevent us from adding anymore inline js!

Stakeholders

@jdlrobson

If you want to help please comment issue for the file you'd like to work on.

jdlrobson avatar Jan 23 '21 18:01 jdlrobson

Hi @jdlrobson , Can I work on books folder?

nynaalekhya avatar Jan 24 '21 06:01 nynaalekhya

May I work on /lib? looks fun :D

navneetsaluja avatar Jan 24 '21 21:01 navneetsaluja

Hi @jdlrobson , Can I work on books folder?

Perhaps openlibrary/templates/books/edit/ - hopefully that will only equate to the edit interface for books.

May I work on /lib? looks fun :D

lib/analytics runs on every page, so should be relatively straightforward to test so perhaps start there?

jdlrobson avatar Jan 24 '21 23:01 jdlrobson

@jdlrobson, I would like to give it a shot. Can I work on openlibrary/templates/subjects.html?

SaravgiYash avatar Jan 26 '21 06:01 SaravgiYash

I can give the /covers folder a try if you'd like @jdlrobson. Let me know if there's any particular file I would want to start with!

drakene avatar Feb 08 '21 16:02 drakene

I think manage would be a good place to start.

jdlrobson avatar Feb 08 '21 17:02 jdlrobson

@jdlrobson , may I work on this issue? I would like to help clean up the inline JS for openlibrary/templates/books/check.html.

CliftonMcCallum avatar Feb 18 '21 18:02 CliftonMcCallum

Just Curios about this Issue, Would not it will be messier if all the js file will be put inside one file, Though I want to help in one of the fie as well, can I work on covers folder

ArunTeltia avatar Mar 06 '21 10:03 ArunTeltia

The motivation is to get all the javascript in one place to make us less confident making changes there. Having code in one place also allows us to reuse and refactor our code.

In javascript we can import JavaScript so the intention is for large blocks of JavaScript they will live in their own file and be imported asynchronously where needed.

jdlrobson avatar Mar 06 '21 18:03 jdlrobson

Now that covers is done, I can move to work on the admins folder next @jdlrobson. I don't think I see anyone who has jumped on it yet.

drakene avatar Mar 16 '21 15:03 drakene

@yashs911 @nynaalekhya how are you getting on with your parts of this task?

jdlrobson avatar Mar 16 '21 22:03 jdlrobson

@Yashs911 @nynaalekhya how are you getting on with your parts of this task?

@jdlrobson sorry for the late reply. How should I go about removing this: <script type="text/json+graph" id="graph-json-chartPubHistory">$:json_encode(page.publishing_history)</script>

SaravgiYash avatar Mar 19 '21 14:03 SaravgiYash

@Yashs911 @nynaalekhya how are you getting on with your parts of this task?

@jdlrobson sorry for the late reply. How should I go about removing this: <script type="text/json+graph" id="graph-json-chartPubHistory">$:json_encode(page.publishing_history)</script>

There is no need to remove this. This is not inline Javascript. Inline JSON is fine.

jdlrobson avatar Mar 19 '21 21:03 jdlrobson

@jdlrobson, I'll have a look at openlibrary/templates/work_search.html.

lephemere avatar Apr 19 '21 10:04 lephemere

I'll take openlibrary/templates/type/user/edit.html

RayBB avatar Apr 25 '21 07:04 RayBB

@Yashs911 how are you getting on with the subjects page?

Is anyone interested in tackling the other files? We are closing in on our goal of one JavaScript entry point to rule them all :) !

jdlrobson avatar May 01 '21 18:05 jdlrobson

I'll have a look at openlibrary/templates/books/edit/web.html.

lephemere avatar May 02 '21 10:05 lephemere

I'll continue on the books edit templates: openlibrary/templates/books/edit/addfield.html

lephemere avatar May 02 '21 17:05 lephemere

I am working on openlibrary/templates/books/edit/edition.html as part of #395 .

The autocomplete stuff is quite a pain so far

RayBB avatar May 02 '21 22:05 RayBB

I'll also take on openlibrary/templates/books/author-autocomplete.html since it's related to work I did for the edition edit page. But I'll wait until I get some feedback on the above three PRs.

RayBB avatar May 16 '21 20:05 RayBB

Thanks for pushing on this one!

jdlrobson avatar May 16 '21 21:05 jdlrobson

I started working on openlibrary/templates/books/edit.html.

lephemere avatar May 23 '21 14:05 lephemere

I am working on openlibrary/templates/site/stats.html

SaravgiYash avatar May 23 '21 16:05 SaravgiYash

I'll have a look at openlibrary/templates/type/author/view.html.

lephemere avatar May 27 '21 17:05 lephemere

openlibrary/templates/publishers/view.html can be marked as done via #5188

RayBB avatar May 31 '21 21:05 RayBB

We are getting very close to our goal! @lephemere is looking at openlibrary/templates/books/edit/addfield.html and openlibrary/templates/type/author/view.html

I'm looking for help with the remaining tasks here.

Beginner/good first tasks:

  • openlibrary/macros/LoanStatus.html (move code into index.js verbatim)
  • openlibrary/macros/TypeChanger.html (move global into index.js along with the thinginput onchange event registration)
  • openlibrary/templates/admin/case.html (possibly dead code that can be deleted after a few discussions in Slack/testing)
  • openlibrary/templates/lib/edit_head.html (conditional check for existence of #editHead element, inside index.js)
  • openlibrary/templates/account/borrow.html (requires communicating from server to client when 5 loans are present via a data-loan-total attribute. The date enhancement can be moved into index.js)

Intermediate tasks:

  • ~~openlibrary/templates/history.html (Will require investigation into what it's doing and a possible no JS equivalent)~~
  • i18n: (openlibrary/templates/type/i18n/edit.html, openlibrary/templates/type/i18n/view.html - requires moving some onclick attributes to a conditional javaScript file that progressively enhances the elements using jQuery)

Harder (but rewarding!) tasks for those who want it!:

  • #2468 - challenging but should be very rewarding! ( openlibrary/templates/type/list/view.html , openlibrary/templates/lists/widget.html , openlibrary/templates/lists/lists.html)
  • openlibrary/templates/account/readinglog_stats.html (use data-config on div.readinglog-charts element instead)
  • openlibrary/templates/site/head.html (this must be done last, and will also allow us to add the async attribute to our JavaScript which should result in a huge performance win. Person who works on this task is also responsible for kicking off the Slack celebration thread :)

jdlrobson avatar Jun 02 '21 05:06 jdlrobson

openlibrary/templates/history.html (Will require investigation into what it's doing and a possible no JS equivalent)

It basically selects two latest revision radio inputs (a10 and b11) and if only one revision is present then it selects (a1, b1) image

SaravgiYash avatar Jun 02 '21 14:06 SaravgiYash

Does anybody have bandwidth to look at openlibrary/templates/account/readinglog_stats.html or openlibrary/templates/type/i18n/edit.html and openlibrary/templates/type/i18n/view.html ?

jdlrobson avatar Jun 25 '21 04:06 jdlrobson

@jdlrobson I can do, I'll share the results

tuminzee avatar Feb 26 '22 14:02 tuminzee

@jdlrobson looks like these can be checked off:

  • openlibrary/templates/lists/lists.html via #2468
  • openlibrary/templates/type/list/view_body.html via #2468
  • openlibrary/templates/type/author/view.html via #5262
  • openlibrary/templates/account/borrow.html via #5644

And soon:

  • openlibrary/templates/admin/case.html via #8260
  • openlibrary/templates/books/edit/addfield.html via #8261

Curious if you or anyone has an idea of what this does: https://github.com/internetarchive/openlibrary/blob/f7388d603bb748771529c68c0558b6718211fc3b/openlibrary/templates/lib/edit_head.html#L3-L9

We don't seem to have similar code anywhere else in the codebase. and I'm not sure what would trigger it. My only guess is that it's somehow related to iframes.

Edit: discussion here said maybe it's an old security measure.

RayBB avatar Sep 01 '23 22:09 RayBB