Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.0] Kill grunt! #18446

Merged
merged 11 commits into from
Jan 17, 2018
Merged

[4.0] Kill grunt! #18446

merged 11 commits into from
Jan 17, 2018

Conversation

dgrammatiko
Copy link
Contributor

@dgrammatiko dgrammatiko commented Oct 30, 2017

Pull Request for Issue # .

Summary of Changes

Two years ago (or so) I did this Grunt file with the aim to lift some of the day to day tasks for maintainers and also provide an easy way for contributors to compile scss files and minify js and css.
As the project (J4) moved on we added a lot more functionality and the file reached about 800 lines of code, which is kinda awkward for a task runner file.
Also we already encounter (a few times) that plugins will fall out of sync with the latest releases of the underlaying scripts (postfix, uglify, etc).
Therefore I propose a a better (for the long term) solution: plain node js code, no Grunt, no Gulp, no Webpack or Rollup. The tasks are pretty straight forward and we're not doing any packaging so webpack and rollup are totally out of the picture.
Also we are now using the package.json as the only point of truth (used to be another yaml file)

Available Commands

  • Compile css
node build --compilecss
  • Prepare the installation language.js
node build --installer
  • Compile/compress js
node build --compilejs
  • Update all vendor dependencies
node build --update

Expected result

Same functionality as Grunt (some command)

Documentation Changes Required

For documentation just run

node build --help

or just

node build

Enjoy!

Preview

screen shot 2017-10-30 at 13 19 11

@dgrammatiko
Copy link
Contributor Author

I forgot to mention that:
compilecss and compilejs options produce ALWAYS the same output, compared to the mess we had in grunt!

@C-Lodder
Copy link
Member

You appear to be loading the wrong jQuery UI package from NPM.

Should be this: https://www.npmjs.com/package/jquery-ui
not this: https://www.npmjs.com/package/jquery-ui-dist

@C-Lodder
Copy link
Member

Also, can we kill the uncompressed CSS files please? We're not using them at all.

package.json Outdated
{
"targets": {
"browsers": [
"last 2 versions",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be last 1 version

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one is for babel, not postcss


// Update the XML files for Codemirror
let codemirrorXml = fs.readFileSync(rootPath + '/plugins/editors/codemirror/codemirror.xml', {encoding: 'UTF-8'});
codemirrorXml = codemirrorXml.replace(xmlVersionStr, "$1" + options.dependencies.tinymce + "$3");
Copy link
Member

@C-Lodder C-Lodder Oct 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

options.dependencies.tinymce should be options.dependencies.codemirror as you've reverted Codemirror back to 4.7.1 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops

@mbabker
Copy link
Contributor

mbabker commented Oct 30, 2017

Also, can we kill the uncompressed CSS files please? We're not using them at all.

Unless you're going to kill support for the core templates and core media loading uncompressed files in debug mode they should stay.

@C-Lodder
Copy link
Member

C-Lodder commented Oct 30, 2017

@mbabker - Atum doesn't have any uncompressed CSS files and works perfectly fine in debug mode: https://github.com/joomla/joomla-cms/tree/4.0-dev/administrator/templates/atum/css

@mbabker
Copy link
Contributor

mbabker commented Oct 30, 2017

I know it all works fine in debug mode. JHtml only uses uncompressed if it can take a .min.foo filename, convert it to .foo, and find a file that exists. So if uncompressed media doesn't exist then the compressed file still gets loaded.

This might be OK for the template media, but it should still be possible to get the core media in uncompressed format. Which means either we keep them in repo & ship them or people have to go through extra steps to compile them.

@Fedik
Copy link
Member

Fedik commented Nov 2, 2017

@DGT41 it is still WIP?

I got error:
screen 2017-11-02 17 55 32 970x275

@Fedik
Copy link
Member

Fedik commented Nov 2, 2017

@DGT41 ignore me, all works fine

@Fedik
Copy link
Member

Fedik commented Nov 3, 2017

@DGT41 one note:

       "srcjs": "dist",
       "srccss": "dist",
        "filesjs": [
          "cropper.js",
          "cropper.min.js",
          "cropper.js.map"
        ],

I found it confusing, because you have "srcjs": "dist", and dist usually contain distributive files, not source 😉

And question.
Is there no way to get the module path, and scan for available files, automatically?
something like :

var pathToModule = require.resolve('moduleBlabLa');
var files = scanforJsCss(pathToModule);

@dgrammatiko
Copy link
Contributor Author

@Fedik that's an interesting idea, I have to explore that

@C-Lodder
Copy link
Member

C-Lodder commented Nov 17, 2017

Could we get the conflicts on this fixed please and then merge?

Any improvements can be made at a later date

@dgrammatiko
Copy link
Contributor Author

Grrrr
screen shot 2017-12-31 at 11 24 01

@C-Lodder
Copy link
Member

Just recompile and remove the grunt-settings.yaml

@dgrammatiko
Copy link
Contributor Author

@wilsonge important please merge this!!

@C-Lodder
Copy link
Member

C-Lodder commented Jan 2, 2018

@wilsonge please merge. This is a choice made by the JS team and will also assist the Ciaran and myself who are the primary people making use of the feature.

@dgrammatiko
Copy link
Contributor Author

@wilsonge any decision here, before you merge more things and make this harder to get it again in sync?

@wilsonge
Copy link
Contributor

Fix the conflicts and I'll merge it. I'm very uneasy about this to be honest. I still think gulp might be better for us. But let's just get it in so it's not blocking you guys

@dgrammatiko
Copy link
Contributor Author

This is in-sync again. Ping @wilsonge

@wilsonge
Copy link
Contributor

You need to fix travis please

@dgrammatiko
Copy link
Contributor Author

Travis is happy again

@wilsonge wilsonge merged commit 122281a into joomla:4.0-dev Jan 17, 2018
@wilsonge wilsonge added this to the Joomla 4.0 milestone Jan 17, 2018
@dgrammatiko dgrammatiko deleted the §4.0-dev-kill-grunt branch January 17, 2018 21:24
@dgrammatiko
Copy link
Contributor Author

Thank you!

brianteeman pushed a commit to brianteeman/joomla-cms that referenced this pull request Jan 18, 2018
* [4.0] Remove grunt in favour of native node build scripts (joomla#18446)

* These are junk

* First pass on converting JS

* Fix path to loader

* CS tweaks for IFW JS, move event listener registration into success callback so that the changed DOM elements correctly receive them, change how custom events are triggered so they'll actually work

* Make sure elements exist in DOM
@infograf768
Copy link
Member

@DGT41
I'm afraid this has broken, among other stuff, the radio field on Firefox.
See #19403

@C-Lodder
Copy link
Member

We are in the process pf fixing things

wilsonge pushed a commit that referenced this pull request Jan 20, 2018
* [4.0] Remove grunt in favour of native node build scripts (#18446)

* These are junk

* First pass on converting JS

* Fix path to loader

* CS tweaks for IFW JS, move event listener registration into success callback so that the changed DOM elements correctly receive them, change how custom events are triggered so they'll actually work

* Make sure elements exist in DOM
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants