Support custom type attribute in scriptLoader method with new parameter T/5434 #5435
Conversation
Support custom type attributes on the rendered script tag using a new typeAttribute parameter that defaults to 'text/javascript'.
Test that the new custom type attribute is detected using the new typeAttribute parameter on the scriptLoader.load method.
There was a problem hiding this comment.
Hi, @matthandus , and thanks for your contribution!
The solution you propose works well if the load() method gets only one script URL to load. However, it can also take an array of scripts:
CKEDITOR.scriptLoader.load( [
'script1.js',
'script2.js',
'script3.js'
] );How would the typeAttribute parameter work in such a case?
Additionally, I'm wondering about the use cases of loading modules via the CKEditor's script loader instead of creating the `script` element in vanilla JS – could you provide an example?
| * @param {String} [typeAttribute] Set a custom type attribute on the | ||
| * script tag. Defaults to `text/javascript`. |
There was a problem hiding this comment.
To indicate the default value of the parameter, please use the [key=value] syntax – it's automatically recognized by our documentation generator and transformed into appropriate information about the default value:
| * @param {String} [typeAttribute] Set a custom type attribute on the | |
| * script tag. Defaults to `text/javascript`. | |
| * @param {String} [typeAttribute='text/javascript'] Set a custom `type` attribute on the | |
| * script tag. |
| if ( !typeAttribute ) | ||
| typeAttribute = 'text/javascript'; |
There was a problem hiding this comment.
In newer code we use braces. There are absent in this file due to its age (in older code we were often omitting them):
| if ( !typeAttribute ) | |
| typeAttribute = 'text/javascript'; | |
| if ( !typeAttribute ) { | |
| typeAttribute = 'text/javascript'; | |
| } |
| @@ -25,6 +25,23 @@ var tests = { | |||
| this.wait(); | |||
| }, | |||
|
|
|||
| 'test load with custom type attribute': function() { | |||
There was a problem hiding this comment.
The test fails in all browsers. The first reason is the usage of the script.$.attr() method. The script.$ property points to the native script element, so to get its type you can use script.$.type.
However, even after this change, the test still fails – probably due to the fact that the ../_assets/sample.js script is already added to the document and the findOne() method gets the first added script element with it. You should check the latest added script.
Additionally, this test won't work in Internet Explorer as it won't preserve the module value of the type attribute. I'm not sure if it's possible to create a test that would work there. Could you check it and – if it's not possible – ignore the test there? For ignoring you could use a code similar to the one below:
if ( CKEDITOR.env.ie ) {
assert.ignore();
}
|
It's been a while since we last heard from you. We are marking this pull request as stale due to inactivity. Please provide the requested feedback or the pull request will be closed after next 7 days. |
github-actions
bot
added
the
stale
|
Hello, just reviewing the feedback now. I will respond soon. Thank you! |
github-actions
bot
removed
the
stale
|
It's been a while since we last heard from you. We are marking this pull request as stale due to inactivity. Please provide the requested feedback or the pull request will be closed after next 7 days. |
github-actions
bot
added
the
stale
|
There was no activity regarding this pull request in the last 14 days. We're closing it for now. Still, feel free to provide us with requested feedback so that we can reopen it. |
github-actions
bot
added
the
pr:frozen ❄
What is the purpose of this pull request?
New feature to support a custom type attribute on the rendered script tag from the method CKEDITOR.scriptLoader.load to better integrate with applications that may need this, like Drupal.
<script src="/path/to/script.js" type="module"></script>Does your PR contain necessary tests?
New test named 'test load with custom type attribute' in /tests/core/scriptloader.js.
This PR contains
Did you follow the CKEditor 4 code style guide?
Your code should follow the guidelines from the CKEditor 4 code style guide which helps keep the entire codebase consistent.
What is the proposed changelog entry for this pull request?
What changes did you make?
Added a new parameter named 'typeAttribute' to the load method on CKEDITOR.scriptLoader that is default to 'text/javascript'. This will allow Drupal developers to set the custom type attribute of 'module' on the script tag when needed. Also added a new test to confirm the custom parameter works.
Which issues does your PR resolve?
Closes #5434.