Skip to content

Conversation

@dgrammatiko
Copy link
Contributor

@dgrammatiko dgrammatiko commented Jan 26, 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

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.0-dev labels Jan 26, 2021
@krileon
Copy link

krileon commented Jan 26, 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.

@brianteeman
Copy link
Contributor

Some filename changes

Where?

@dgrammatiko
Copy link
Contributor Author

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

@brianteeman
Copy link
Contributor

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

@krileon
Copy link

krileon commented Jan 26, 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.

@dgrammatiko
Copy link
Contributor Author

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.

@dgrammatiko
Copy link
Contributor Author

@krileon fixed with 5babcd4

@brianteeman
Copy link
Contributor

@dgrammatiko
Copy link
Contributor Author

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

ok will add all the affected files there

@Fedik
Copy link
Member

Fedik commented Jan 26, 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

@krileon
Copy link

krileon commented Jan 26, 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.

@dgrammatiko
Copy link
Contributor Author

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

@Fedik
Copy link
Member

Fedik commented Jan 26, 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.

@dgrammatiko
Copy link
Contributor Author

dgrammatiko commented Jan 26, 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...

@krileon
Copy link

krileon commented Jan 26, 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.

@dgrammatiko
Copy link
Contributor Author

@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 😉

@dgrammatiko
Copy link
Contributor Author

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

@krileon
Copy link

krileon commented Jan 26, 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.

@Fedik
Copy link
Member

Fedik commented Jan 27, 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.

@dgrammatiko
Copy link
Contributor Author

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?

@Fedik
Copy link
Member

Fedik commented Jan 27, 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.

@dgrammatiko
Copy link
Contributor Author

dgrammatiko commented Jan 27, 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

@dgrammatiko
Copy link
Contributor Author

dgrammatiko commented Jan 27, 2021

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

@Fedik
Copy link
Member

Fedik commented Jan 27, 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.

@dgrammatiko
Copy link
Contributor Author

Aha, I will fix the definitions later on today

@krileon
Copy link

krileon commented Jan 27, 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.

@wilsonge wilsonge merged commit 6eeac35 into joomla:4.0-dev Jan 28, 2021
@wilsonge wilsonge added this to the Joomla 4.0 milestone Jan 28, 2021
@Fedik
Copy link
Member

Fedik commented Jan 28, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

NPM Resource Changed This Pull Request can't be tested by Patchtester

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants