Clean-up JS code in UI templates #14019
Conversation
- Use template literals instead of '+' for forming strings, when applicable - remove unused variables (gantt.html) - remove unused function arguments, when applicable
XD-DENG
added
priority:low
XD-DENG
requested review from
ryanahamilton,
ashb,
potiuk,
kaxil,
turbaszek and
mik-laj
|
All these are very simple changes. But it would be great to have more pairs of eyes to help check in case any mistake is made. I understand @ryanahamilton is working on automated eslint integration, but I guess it will come only later, so I hope this change is still helpful. |
| @@ -375,13 +375,13 @@ <h4 class="modal-title" id="dagModalLabel"> | |||
| {{ super() }} | |||
| <script> | |||
| function updateQueryStringParameter(uri, key, value) { | |||
| var re = new RegExp('([?&])' + key + '=.*?(&|$)', 'i'); | |||
| var re = new RegExp(`([?&])${key}=.*?(&|$)`, 'i'); | |||
There was a problem hiding this comment.
What browsers support this? It's a "relatively" new syntax (and JS-in-html is not subject to any compile steps)
There was a problem hiding this comment.
Do you mean the template literals? Here is the information: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals#browser_compatibility Only IE is marked "No support"
This should not be a concern to me though, because there is already usage of template literals in 2.0.0 (example And I believe in some 1.10.* versions too) and so far there is not any complaint regarding this.
Let me know if you are referring to anything else?
There was a problem hiding this comment.
Ideally, we'd start migrating these scripts to external (compiled) files to make this concern moot. I'm not super concerned about IE support, but I wish we had actual usage data to better inform us. Google Analytics appears to be installed on the documentation site—maybe its browser usage reports could serve as proxy (better than nothing)?
There was a problem hiding this comment.
I conferred w/ @ashb and we're both good on moving forward with this given it is already in practice. I'll add an issue for migrating all of the inline scripting to external files.
|
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease. |
github-actions
bot
added
the
okay to merge
|
Hi @ryanahamilton @ashb , may I know if there is already an issue created for items below?
Let me know if not, and I can create it (surely will need you to help enrich the description though). |
|
@XD-DENG no, not yet. That would be appreciated, and I can certainly add more detail. As for the ESLint integration, I actually made an attempt at resolving it several months ago in #11699. While I had it working locally with pre-commit, I was unable to get it executing properly in our CI environment. |
|
I think Kamil may have created an issue about splitting it out a year+ ago but can't find it right now |
|
I have tried to search now as well, but cannot find any either. I will proceed to create one; if we find it duplicated later, we can merge & clean anyway. |
|
I created a new issue at #14115 . Please feel free to enrich/correct the description. In terms of timeline, does aiming for having in 2.2 make sense? @ashb @ryanahamilton . Let's continue the discussion in the issue anyway. Thanks. |
Clean up JS code in UI templates. Mainly:
gantt.html)^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.