openlibrary icon indicating copy to clipboard operation
openlibrary copied to clipboard

remove unused utils

Open RayBB opened this issue 2 months ago • 5 comments

Closes #9179

Technical

Testing

Did a fulltext search of the code and these aren't used anywhere I can see but a second check would be good. Removes one unused piece of code and adds a comment about why we want to keep the rest.

Screenshot

Stakeholders

RayBB avatar Apr 29 '24 01:04 RayBB

Howdy folks, I've been thinking about this one and I think it might be best for us to close this. Even if we remove the places where our jsdef code uses e.g. cond, it's expected that jsdef codeblocks should have access to these python globals. Removing the JS implementations here will make our code brittle, especially since we don't really have strong tests to verify that our python/jsdef templates are valid.

cdrini avatar May 13 '24 14:05 cdrini

To clarify, here is what I mean by jsdef using these files:

https://github.com/internetarchive/openlibrary/blob/1860c2562819330bf4657f8ed13d2c3d0ea336e2/openlibrary/templates/subjects.html#L84-L91

The cond call here uses the window.cond definition when this template is converted to/called from the JS.

cdrini avatar May 13 '24 15:05 cdrini

@cdrini thanks for replying.

Should we also keep focusNextInputField ?

Do we need to keep the window.cond = cond; for jsdef to work? Or would it be feasible to move these to a window.jsdef.cond so they're not taking up so much at the top of index.js?

RayBB avatar May 13 '24 15:05 RayBB

focusNextInputField looks entirely unused; that looks like we can drop :+1: We do need the window.cond exposure in order for jsdef to work atm; if we wanted to change that, we would need to modify the way that the jsdef converts the templates to JS to check a certain global eg a JSDEF global. That would be nice in some future! But a little complicated to achieve. We'd need to change the parser to basically alter all globals to pull them from somewhere else. Here: https://github.com/internetarchive/openlibrary/blob/b46ff5135473c003f191d804bdd3b07cfa2c37cb/openlibrary/plugins/upstream/jsdef.py

cdrini avatar May 13 '24 15:05 cdrini

@cdrini sounds good to me. I've pushed code with a comment about this and removing focusNextInputField

RayBB avatar May 13 '24 15:05 RayBB