[#3111] - Using Punycode.js as a named module in RequireJS
- Closed
- 17 Feb 2014
- Medium
- Build: staging
- # 3111
- Diff
- anibalsanchez:staging
- Foreign ID 33105
- Success The Travis CI build passed Details
User tests: Successful: Unsuccessful:
Punycode.js has conditional code to declare itself as a RequireJS module (when RequireJS is previously loaded).
If other scripts declare RequireJS named modules, Punycode.js causes this error:
Error: Mismatched anonymous define() module: function () ...
To avoid this conflict, all RequireJS modules in Joomla core must be named modules.
In this case, the PR just defines Punycode name as 'punycode'.
define('punycode', function() ...
The change does not affect the regular form validation. (http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=33105&start=0).
It only defines the module name if RequireJS is loaded.
Joomlacode: http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=33277&start=0
Punycode developers can have a different opinion about named modules. It's a general practice to define un-named modules.
In the other hand, in Joomla, we can adopt a safe practice of naming modules to have a common standard for a future RequireJS adoption.
If not, you are forcing whom are currently using RequireJS to avoid Joomla form validation, or implement custom Javascript loading at the end of the head/body.
My point is that, generally speaking, we do not modify 3rd party libraries as these may need to be updated when a new version of the library is released. In that case this change could be forgotten.
I agree with the general rule, but in this case punycode causes conflicts in a shared environment, where several actors can load scripts.
It's better if we use Javascript named modules practice. All JS modules in Joomla must have a defined name.
PD: We have already customized the whole Bootstrap to be friendly with MooTools modals. #1172
Thanks for the report. You’re absolutely right, it makes no sense to do this for Punycode.js as we’re not trying to alias a competitor (contrary to e.g. Lo-Dash & Underscore.js). This is now fixed upstream in Punycode.js v1.2.4.
The reasoning matches that of jashkenas/underscore@d12b5be.
Thanks @matiasbynens for replying so fast to my mail :)
As the original library was now corrected, we can now update it (with new version and including the (c)
A there were other small changes I merged directly the new library (after testing :)
Thanks @anibalsanchez and Mathias
| Status | New | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2014-02-17 08:30:40 |
Cool!
As this is a change in the library itself, I suggest to rather propose it to the punycode original coder. Once accepted and the library updated by him, we should consider it.