NPM Resource Changed ? Pending

User tests: Successful: Unsuccessful:

avatar dgrammatiko
dgrammatiko
26 Jan 2021

Pull Request for Issue # .

Summary of Changes

  • Drop the polyfill for Custom Events
  • Drop the polyfill for Web Components
  • Some filename changes
  • Adjust all the calls to reflect the type=module

Testing Instructions

Test various views in the backend and confirm nothing is broken

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

Documentation Changes Required

avatar dgrammatiko dgrammatiko - open - 26 Jan 2021
avatar dgrammatiko dgrammatiko - change - 26 Jan 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 26 Jan 2021
Category Administration com_associations com_categories com_config com_fields com_installer com_media com_modules JavaScript Repository NPM Change Installation Layout
avatar dgrammatiko dgrammatiko - change - 26 Jan 2021
Labels Added: NPM Resource Changed ?
avatar krileon krileon - test_item - 26 Jan 2021 - Tested successfully
avatar krileon
krileon - comment - 26 Jan 2021

I have tested this item successfully on 9a0d464


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/32171.

avatar brianteeman
brianteeman - comment - 26 Jan 2021

Some filename changes

Where?

avatar dgrammatiko
dgrammatiko - comment - 26 Jan 2021

Where?

It happens in the build steps, check build/build-modules-js/javascript/compile-w-c.es6.js and build/build-modules-js/settings.json

avatar brianteeman
brianteeman - comment - 26 Jan 2021

So we probably need to add the old files to the list of files to be removed for those people updating

avatar krileon
krileon - comment - 26 Jan 2021

CodeMirror broken in d4d9619

Before:
j4_codemirror_pre_commit
After:
j4_codemirror_post_commit

No JS errors. Just seams like it's not being bound to the element at all.

avatar dgrammatiko
dgrammatiko - comment - 26 Jan 2021

So we probably need to add the old files to the list of files to be removed for those people updating

Updating from J3 is not affected, between betas I have no clue how this is handled.

avatar dgrammatiko
dgrammatiko - comment - 26 Jan 2021

@krileon fixed with 5babcd4

avatar brianteeman
brianteeman - comment - 26 Jan 2021
avatar dgrammatiko
dgrammatiko - comment - 26 Jan 2021

between betas is the same as 3.x to 3.x+1

ok will add all the affected files there

avatar Fedik
Fedik - comment - 26 Jan 2021

Well, it does not need to rename everything, that not productive, and probably will lead to bugs.
It will work perfectly fine with existing names

avatar krileon krileon - test_item - 26 Jan 2021 - Tested successfully
avatar krileon
krileon - comment - 26 Jan 2021

I have tested this item successfully on 5babcd4

CodeMirror and None editors tested working now.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/32171.

avatar dgrammatiko
dgrammatiko - comment - 26 Jan 2021

It will work perfectly fine with existing names

I like to get everything consistent. Right now it's a mess. Let's keep the .es6.js for the time being, I mean we literally just shipped BS based on that convention:

Screenshot 2021-01-26 at 20 56 58

avatar Fedik
Fedik - comment - 26 Jan 2021

Well, then, with all this renaming it not acceptable at beta stage, to me. Sorry.

All was need is just disable transpiller and drop WC loader in core.js.

avatar dgrammatiko
dgrammatiko - comment - 26 Jan 2021

All was need is just disable transpiller and drop WC loader in core.js.

That's not what this PR is doing. It just aligns the idiomatic (different and odd) web components names to the rest of the scripts and also treats them as modules (totally fine, there's just a class in there). Why is it not acceptable?

PS I stand corrected if the target is ES6 to be the .min.js this is a step backwards...

avatar krileon
krileon - comment - 26 Jan 2021

All was need is just disable transpiller and drop WC loader in core.js.

Wouldn't that be a second step to accomplish after this PR? I believe we discussed best course of action is a step by step approach instead of some monolithic PR. @dgrammatiko states as much here.

avatar dgrammatiko
dgrammatiko - comment - 26 Jan 2021

@chnnst would be more helpful if you tested the PR and log it here: https://issues.joomla.org
Don't get me wrong but likes are not really helpful here ?

7c5e485 26 Jan 2021 avatar dgrammatiko Init
avatar joomla-cms-bot joomla-cms-bot - change - 26 Jan 2021
Category Administration com_associations com_categories com_config com_fields com_installer com_media com_modules JavaScript Repository NPM Change Installation Layout JavaScript Repository NPM Change Libraries Front End Plugins
avatar dgrammatiko
dgrammatiko - comment - 26 Jan 2021

@Fedik @krileon This was redone so it has only the minimal required changes for the task in hand

avatar krileon
krileon - comment - 26 Jan 2021

@Fedik @krileon This was redone so it has only the minimal required changes for the task in hand

Ok, I'll be able to retest tomorrow. Thank you for doing all of this.

3b46e06 26 Jan 2021 avatar dgrammatiko defer
avatar joomla-cms-bot joomla-cms-bot - change - 26 Jan 2021
Category JavaScript Repository NPM Change Libraries Front End Plugins Repository NPM Change Libraries Front End Plugins
avatar dgrammatiko dgrammatiko - change - 26 Jan 2021
The description was changed
avatar dgrammatiko dgrammatiko - edited - 26 Jan 2021
avatar dgrammatiko dgrammatiko - change - 26 Jan 2021
The description was changed
avatar dgrammatiko dgrammatiko - edited - 26 Jan 2021
avatar Fedik
Fedik - comment - 27 Jan 2021

Thanks! that looks much better.

webcomponent prefix

There 2 reason:

  • This is documented in this way,
  • People who already updated their extensions for J4 and use core webcomponents for sure will use webcomponent.blabla. And any renaming will break it. (remember there a couple betas already out)

file names

This for future updates.
It better to use "final file name" (without suffix) to avoid future confusion with renaming, because after we fully move to es6 this suffix will make no sense. And all legacy files should have a suffix.

The same for other scripts. If we going to ship es6 files as default then they should be called without suffix, and all legacy files should have a suffix. So in future we just drop "legacy" files without much trouble.

avatar dgrammatiko
dgrammatiko - comment - 27 Jan 2021

If we going to ship es6 files as default then they should be called without suffix, and all legacy files should have a suffix

Agreed. This way we will not repeat the odd -uncompressed.js/.js thingy we had all over the place in J3.

One question is it possible to have a dependency loaded before core.js? If so how?

avatar Fedik
Fedik - comment - 27 Jan 2021

it possible to have a dependency loaded before core.js?

Not for regular script.

For inline it possible:

$wa->addInlineScript('alert("aaa!")', ['position' => 'before'], [], ['core']);

This kind of reverse dependency I guess. A couple people already asked about same, maybe need to think something in future for regulars script.

avatar dgrammatiko
dgrammatiko - comment - 27 Jan 2021

A couple people already asked about same, maybe need to think something in future for regulars script

Yeah, some array reordering function could be helpful. Anyways it turns out I don't need it here.

@wilsonge this should be almost the same behaviour as before:

  • Browsers without script module support will load the polyfills (all of them shadow dom, template, custom element, etc)
  • Browsers with script module support just load the ES6 code

One image is 1000 words
Screenshot 2021-01-27 at 10 55 46

b134057 27 Jan 2021 avatar dgrammatiko b/c
avatar dgrammatiko
dgrammatiko - comment - 27 Jan 2021

@Fedik the dependencies are not rendering in the expected order (or at least that wasn't my expectation), check the image above

avatar Fedik
Fedik - comment - 27 Jan 2021

It is correct order from how it configured.
if you want the legacy script to be after "wcpolyfill", then legacy script should have "wcpolyfill" dependency, main script does not require it .
Something like:

alert-legacy => depend from "wcpolyfill"
alert => depend from "alert-legacy"

btw, if you will use nomodule: true then it should render attribute like nomodule, instead of nomodule="". If not, then it a bug I guess.

avatar dgrammatiko
dgrammatiko - comment - 27 Jan 2021

Aha, I will fix the definitions later on today

avatar krileon
krileon - comment - 27 Jan 2021

It's completely unnecessary to add polyfills for IE support. IE is not supported as is. IE is also being completely discontinued (EOL) in August by Microsoft. This is a lot of extra work for something we can't even test (J4 frontend and backend do not work in IE) for a literal dead browser.

avatar wilsonge wilsonge - change - 28 Jan 2021
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2021-01-28 10:34:17
Closed_By wilsonge
avatar wilsonge wilsonge - close - 28 Jan 2021
avatar wilsonge wilsonge - merge - 28 Jan 2021
avatar Fedik
Fedik - comment - 28 Jan 2021

@wilsonge I think you a bit to fast here, @dgrammatiko still need to fix dependency issue ?
but can be done in another PR

Add a Comment

Login with GitHub to post a comment