[#43758] - [5.3] [bugfix] Use iife for the scripts core and load validate as module
- Fixed in Code Base
- 7 Dec 2024
- Medium
- Build: 5.1-dev
- # 43758
- Diff
- dgrammatiko:iife51
User tests: Successful: Unsuccessful:
Pull Request for Issue #43714 .
Summary of Changes
Use iife instead of esm for the script core.js and load the script validate.js with a type=module
Testing Instructions
- Apply the PR
- run
npm install - Check that authenticating in the administrator works
- Create a new article, don't add anything and press save, the validation error should appear
Actual result BEFORE applying this Pull Request
Leaking variables in the global scope
Expected result AFTER applying this Pull Request
NO leaking variables in the global scope
Link to documentations
Please select:
-
Documentation link for docs.joomla.org:
-
No documentation changes for docs.joomla.org needed
-
Pull Request link for manual.joomla.org:
-
No documentation changes for manual.joomla.org needed
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | JavaScript Repository NPM Change |
| Labels |
Added:
NPM Resource Changed
PR-5.1-dev
|
||
If you have a problems with eslint, just add:
/* global Joomla */
At top of the file. Like we have for TinyMCE
I would suggest to keep for 5.1 the same what you did for 4.4 (will be upmerged).
And set core and validator to type=module for 5.2 as a new feature.
I don't see any problems with the change from defer to type=module. Let me explain:
- the defer script in 4.4 has a
use strictbut with thetype=modulethis is inferred - Both defer and
type=moduleload the script as deferred - The only difference is that in the 4.4 the class
JFormValidatoris globally registered but I could attach it to the window and have the same effect.
In sort this change shouldn't be assumed as a breaking change as it isn't
I'll change the window.Joomla refs
There no problem to set type=module as it is, however they may be extensions that doing "script optimisation" merging non module scripts etc. It will change behavior slightly, to them.
It is not a big deal, but I would keep it safe, and would not to do such changes in patch release.
It is not a big deal, but I would keep it safe, and would not to do such changes in patch release.
I'm willing to take full responsibility for this if the maintainers trust me ?♂️
Then I leave you to maintainers ?
Please add one more globals for window.punycode = punycode; (as it may used by some script).
And then release manages can decide, what to do with the PR.
| Labels |
Added:
RMDQ
|
||
Please add one more globals for window.punycode = punycode; (as it may used by some script).
Done, I guess you could raise this to the maintainers
I have tested this item ✅ successfully on 2c1f421
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43758.
I have tested this item ✅ successfully on 2c1f421
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43758.
| Status | Pending | ⇒ | Ready to Commit |
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43758.
Hey @dgrammatiko thanks for this PR. Just to understand, isn't this a B/C break, if someone e.g. made an override of the "old" core/validation script and now it's loaded as a module?
No, type=module means:
- use strict by default
- Deferred
- Reduced scope
the reduced scope in this case is irrelevant as the classes are exposed as global vars. In sort there won’t be any issue!
Ok, I just saw, that after the upmerge something similar was done in 4.4, could you (@dgrammatiko ) please check, if the changes were correct and if this PR is still needed? Thank you!
| Labels |
Added:
RTC
|
||
| Category | JavaScript Repository NPM Change | ⇒ | Unit Tests Administration com_content com_fields com_finder com_menus com_messages com_redirect com_users Language & Strings JavaScript Repository NPM Change |
| Category | JavaScript Repository NPM Change Unit Tests Administration com_content com_fields com_finder com_menus com_messages com_redirect com_users Language & Strings | ⇒ | JavaScript Repository NPM Change |
| Labels |
Added:
Unit/System Tests
Language Change
|
||
| Labels |
Removed:
Unit/System Tests
Language Change
|
||
| Category | JavaScript Repository NPM Change | ⇒ | Unit Tests Administration com_content com_fields com_finder com_menus com_messages com_redirect com_users Language & Strings JavaScript Repository NPM Change |
| Labels |
Added:
Unit/System Tests
Language Change
|
||
| Category | JavaScript Repository NPM Change Unit Tests Administration com_content com_fields com_finder com_menus com_messages com_redirect com_users Language & Strings | ⇒ | JavaScript Repository NPM Change |
| Labels |
Removed:
Unit/System Tests
Language Change
|
||
@bembelimen these changes are still better than the code in 4.4. ES modules should be the default way of serving js in 2024...
| Status | Ready to Commit | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2024-08-12 12:29:09 |
| Closed_By | ⇒ | dgrammatiko |
| Status | Closed | ⇒ | New |
| Closed_Date | 2024-08-12 12:29:09 | ⇒ | |
| Closed_By | dgrammatiko | ⇒ | |
| Labels |
Removed:
RTC
|
||
| Status | New | ⇒ | Pending |
I rebased this for 5.2, not sure if this comes to 5.2 or not @bembelimen @Hackwar
| Title |
|
||||||
I understand that something similar was added to 4.4 and that theoretically this could go into 5.2, however I feel uncomfortable adding this to a patch release. That's why I'm moving this to 5.3, but I'm also asking @viocassel and @Fedik to re-test and then will merge it into 5.3. Thank you for your work so far.
| Title |
|
||||||
Can you please solve the conflict in the file...
| Labels |
Added:
Feature
PR-5.3-dev
Removed: RMDQ PR-5.1-dev |
||
Can we have here one more test, just to see if it still works as there were some commits after the rebase.
| Status | Pending | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2024-12-07 18:18:02 |
| Closed_By | ⇒ | laoneo |
Thanks!
This is no go, sorry.
I would suggest to keep for 5.1 the same what you did for 4.4 (will be upmerged).
And set core and validator to type=module for 5.2 as a new feature.
Please, without this huge code move, that you made in core.js.
Just
window.Joomla = Joomla;is enough. Having 100window.Joomla.foobar = ..is very bad idea.Thanks.