Skip to content

Conversation

@dgrammatiko
Copy link
Contributor

@dgrammatiko dgrammatiko commented Feb 5, 2021

Pull Request for Issue # .

Summary of Changes

Use type=module for ES2017 and nomodule for ES5 scripts

Testing Instructions

Test every url Front End and Back End to confirm that there's no console errors and all the interactive elements continue to work as before
Especially check the Admin login by insering a username but ignoring the password should raise a message (formbvalidation)
Set the editor to Codemirror and create an article and also check that editing a template file is working as before

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

Documentation Changes Required

Just as a comment here: all .es6.js and .w-c.es6.js are handled by Rollup, meaning that any import will be correctly bundled. Also the compiled -es5.js is again created by Rollup using Babel. This adds a delay on the build command BUT now all the ES6 files are ES2017 (meaning there is translation to a minimum version) against the previous state (ES6 files
could have eg 2021 code that would be problematic for many browsers as the coverage is way bellow the 95% threshold, which is assumed the industry standard for the compatibility lower boundary. Worth watching https://www.youtube.com/watch?v=cLxNdLK--yI by @developit and @housseindjirdeh as this PR basically implements exactly that)

Performance

The impact could be from 1s to 20s depending on the actual hardware

Before

The CI server
Screenshot 2021-02-25 at 13 26 35

My mac
Screenshot 2021-02-25 at 22 22 32

After

The CI server
Screenshot 2021-02-25 at 13 28 16

My mac
Screenshot 2021-02-25 at 22 20 23

Calling for testing @richard67 @Fedik @anibalsanchez @alikon @laoneo

how Joomla is doing the transition

what an extension developer should do to follow the steps

  • If the dev is using Web Assets then the change is a simple switch of the definitions of the scripts (assuming that the source is modern JS and there is a bundler capable of producing ES2017 and ES5 scripts, continue reading).
    Eg legacy script is, let's say some jQuery, so:
    {
       "name": "com_example.awesome-script",
       "type": "script",
       "uri": "com_example/awesome-script.min.js",
       "dependencies": [
         "core"
       ],
       "attributes": {
         "defer": true
       }

Modern way assumes that the script has a source (modern JS) that transpiles to 2 files, one ES2017 and one ES5 (the one with -es5):

    {
       "name": "com_example.awesome-script.es5",
       "type": "script",
       "uri": "com_example/awesome-script-es5.min.js",
       "dependencies": [
         "core"
       ],
       "attributes": {
         "nomodule": true,
        "defer": true
       },
    }
    {
       "name": "com_example.awesome-script",
       "type": "script",
       "uri": "com_example/awesome-script.min.js",
       "dependencies": [
         "com_example.awesome-script.es5"
       ],
       "attributes": {
         "type": "module"
       }
  }
  • If the dev is using the HTMLHelpr then the change is just an additional call.
    Eg: Legacy:
HTMLHelper::_('script', 'com_example/awesome-script.min.js', ['version' => 'auto', 'relative' => true], ['defer' => true]);

Eg Modern:

HTMLHelper::_('script', 'com_example/awesome-script-es5.min.js', ['version' => 'auto', 'relative' => true], ['defer' => true, 'nomodule' => true]);
HTMLHelper::_('script', 'com_example/awesome-script.min.js', ['version' => 'auto', 'relative' => true], ['type' => 'module']);

Mind that the legacy script com_example/awesome-script.min.js was theoretically rewritten as modern JS and was transpiled to ES2017 with the same name and then a transpiled ES5 was produced by a bundler and had a theoretical name com_example/awesome-script-es5.min.js. But this is the convention Joomla is using, devs can use their own naming convention here, Joomla will not impose anything in this part.

What the CMS provides

Joomla has a set of tools for compiling a modern script to ES2017 and ES5 files. Devs can use Rollup, Webpack, Parcel, ESBuild, SnowPack, or any other bundler out there. The scripts for the bundling live at https://github.com/joomla/joomla-cms/tree/4.0-dev/build/build-modules-js/javascript and devs can even use them as-is if they develop on top of the Joomla repo (this is probably an antipattern, but...) or fork them for their own benefit.

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.0-dev labels Feb 5, 2021
@Fedik
Copy link
Member

Fedik commented Feb 5, 2021

looks okau to me.
I think we should note somewhere (at release page or system requirement), that nomodule will be dropped at some point of 4.x, maybe even in 4.1

@dgrammatiko
Copy link
Contributor Author

I think we should note somewhere (at release page or system requirement), that nomodule will be dropped at some point of 4.x, maybe even in 4.1

Sure, but only if we ever come to an agreement on how we move forward with Js, transpiling, etc 🤣

@Fedik
Copy link
Member

Fedik commented Feb 15, 2021

you know what I thinking,
we need es5 fallback only for a scripts that may be used on frontend,
because whole backend is unusable in old browsers anyway (I can actually login, but not much can do).

@dgrammatiko
Copy link
Contributor Author

dgrammatiko commented Feb 15, 2021

because whole backend is unusable in old browsers anyway

The backend is unusable with the default template. If I fork Atum and use bs4 css (patching the changed classes with some extends or whatever) the backend should be still fine on IE11

@wilsonge any input here?

@Fedik
Copy link
Member

Fedik commented Feb 15, 2021

If I fork Atum and use bs4 css

It not only the styling, but much of JS errors (I tried on current 4.0-dev branch, without this PR)

@dgrammatiko
Copy link
Contributor Author

dgrammatiko commented Feb 15, 2021

but much of JS errors

There are known errors right now as people removed some polyfills without our tools adding them back. Basically, the tools are not really transpiling properly the es6 files. Partially this is fixed with #32300 but I think there will be a bit more tweaking required

@Fedik can you post some of those errors?

@Fedik
Copy link
Member

Fedik commented Feb 15, 2021

Basically, the tools are not really transpiling properly the es6 files

yeah I suspect that also,
there was something about undefined require(), and some others,
I can try to run again sometime later and copy some of them, if it makes sense 😄

@dgrammatiko
Copy link
Contributor Author

dgrammatiko commented Feb 18, 2021

there was something about undefined require(), and some others,

@Fedik the require() errors should be fixed with #32459

@Fedik
Copy link
Member

Fedik commented Feb 21, 2021

can you post some of those errors?

here is IE 11 a fresh screenshot of errors, from Dashboard view:
Screenshot_2021-02-21_15-11-36

it not related to current PR, just for info as you asked

@dgrammatiko
Copy link
Contributor Author

dgrammatiko commented Feb 21, 2021

Thanks, so there are quite some problems here:

  • require() comes from a missing rollup plugin fixed with [4.0] Proper cache invalidation of the static assets [.js/.css] #32485
  • closest() right now the the babel is not injecting missing parts AFAIK
  • Object doesn't support property I have no clue, this is also affecting the webcomponent polyfill which is not patched in any way from us, I have to dig this one

EDIT: it seems there's a tag missing: <meta http-equiv="X-UA-Compatible" content="IE=11" />

@dgrammatiko dgrammatiko force-pushed the 4.0-dev__ES_MODULES branch 2 times, most recently from f8b099c to a2b010a Compare February 25, 2021 10:46
@dgrammatiko dgrammatiko marked this pull request as ready for review February 25, 2021 11:49
@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Feb 28, 2021
@dgrammatiko
Copy link
Contributor Author

I have tested, it works. IE11 explodes with errors

I know, I commented out the required lines for IE https://github.com/joomla/joomla-cms/pull/32315/files#diff-7973fa8a952791298a25a8354216ed84a7476bf991fce4be84f05cb1d91f2bce because we might want to abstract (if possible) the Babel runtime (it's 6kb that will be included in each script) but besides that, we need a list of DOM polyfills that Babel is not injecting (eg htmlElement.colsest() ). Also, the webcomponents polyfill should include the file webcomponents-bundle.js instead of bundles/webcomponents-sd-ce-pf.js. In short, this is a bug we can fix, I just didn't want to blow this PR even further. I will do the patches in another PR or PRs

@Fedik
Copy link
Member

Fedik commented Feb 28, 2021

...because we might want to abstract (if possible) the Babel runtime (it's 6kb that will be included in each script) but besides that, we need a list of DOM polyfills that Babel is not injecting

you wanted to say that we need jQuery? 😄

@dgrammatiko
Copy link
Contributor Author

you wanted to say that we need jQuery?

I some cases also Mootools 🤣

@dgrammatiko
Copy link
Contributor Author

@wilsonge can we have a decision here?

@thednp
Copy link
Contributor

thednp commented Mar 7, 2021

@dgrammatiko I think with this PR, it's a good opportunity to change ESlint with @babel/eslint-parser, this one allows static properties to be parsed, something I need for the color-picker called static formAssociated = true;.

@dgrammatiko
Copy link
Contributor Author

@thednp joomla is using the Airbnb rules for eslint and I don’t see why we need to change this. Airbnb is extremely well documented. Also you can’t use formInternals as it is poorly supported

@thednp
Copy link
Contributor

thednp commented Mar 7, 2021

Also onformdata isn't good enough? Seems well supported.

@dgrammatiko
Copy link
Contributor Author

dgrammatiko commented Mar 7, 2021

Safari doesn’t support it so: no

ref https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers/onformdata

@dgrammatiko
Copy link
Contributor Author

Btw I already gave you the solution

@rdeutz rdeutz merged commit 42a70ea into joomla:4.0-dev Mar 10, 2021
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Mar 10, 2021
@dgrammatiko dgrammatiko deleted the 4.0-dev__ES_MODULES branch March 10, 2021 18:40
@zero-24 zero-24 added this to the Joomla 4.0 milestone Mar 10, 2021
@richard67
Copy link
Member

@dgrammatiko Now I have a problem when using node version 12:

2021-03-14_npm-error_1

I.e. it breaks here:

https://github.com/joomla/joomla-cms/blob/4.0-dev/build/build-modules-js/javascript/compile-to-es2017.es6.js#L1

With node version 14 I don't have that problem.

At least one other person could reproduce that, i.e. same problem on same OS (Linux) with node v12, and no problem anymore after removing node and installing v14.

Any idea?

@wilsonge What version of node do we require?

@dgrammatiko
Copy link
Contributor Author

dgrammatiko commented Mar 14, 2021

@richard67 Can you change it to: const { access } = require('fs').promises; ? Does that solve the problem?

Also according to https://nodejs.org/en/about/releases/ v10 goes EOL next month, so v12 should be set as the minimum at the end of April.

@richard67
Copy link
Member

@richard67 Can you change it to: const { access } = require('fs').promises; ?

Not sure if I can. Can you advise me? Does it need a text editor for that?
...
Joking ;-)

Does that solve the problem?

It solves the problem on node v12 on my Linux VM. Will see in a few minutes if it still works with node v14 on my Windows VM.

@dgrammatiko
Copy link
Contributor Author

It solves the problem on node v12 on my Linux VM.

Well, the 'fs/promises' is just a newer syntax of the same thing. Everything should be ok. Node is confusing with the rolling versions but I guess I should have tested this

@richard67
Copy link
Member

Everything should be ok.

It means no need for a fix? Or it means your suggested fix is ok? If the latter: Could you make the necessary PR?

@alikon
Copy link
Contributor

alikon commented Mar 14, 2021

so the question arise what should be the node and npm versions ? for testing of course

@dgrammatiko
Copy link
Contributor Author

sure: #32686

@dgrammatiko
Copy link
Contributor Author

so the question arise what should be the node and npm versions ? for testing of course

Till the end of April v10 should be ok but I guess the project should raise the minimum to v12 before v10 goes EOL. Also whatever you do MARK the date (end of April) as you have to do this every year since Javascript the language gets a new version every year (so nodejs just follows the ecmascript releases)

@brianteeman
Copy link
Contributor

so the question arise what should be the node and npm versions ? for testing of course

joomla-cms/package.json

Lines 10 to 12 in 4bc8bb1

"engines": {
"node": ">=10.19",
"npm": ">=6.13.4"

dgrammatiko added a commit to dgrammatiko/joomla-cms that referenced this pull request Mar 17, 2021
Co-authored-by: Brian Teeman
Co-authored-by: Roland Dalmulder
Co-authored-by: AndySDH
Co-authored-by: Quy
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.