Problem/Motivation

Whilst contributors are responsible for transpiling .es6.js to .js it'll be good to have a command to check if it needs doing. This will help us build automated tools that can fail patches prior to commit so we don't make mistakes.

Proposed resolution

Add a check:js command to package.json that unlike build or watch does not write the files but just fails if the files are not up-to-date.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#13 2880013-2-13.patch6.03 KBalexpott
#13 9-13-interdiff.txt883 bytesalexpott
#9 c20170521_184113.png20.6 KBdroplet
#9 interdiff.patch1.61 KBdroplet
#9 2880013-8.patch5.95 KBdroplet
#7 2880013-2-7.patch5.67 KBalexpott
#7 6-7-interdiff.txt5.14 KBalexpott
#6 interdiff.patch3.34 KBdroplet
#6 2880013-6.patch4.28 KBdroplet
#5 2880013-5.patch4.98 KBalexpott
#5 2-5-interdiff.txt4 KBalexpott
#2 2880013-2.patch3.13 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Active » Needs review
FileSize
2880013-2.patch3.13 KB

I think we might be able to refactor the tooling there is less duplication between this and the other commands... maybe a js expert can help :)

droplet’s picture

droplet CreditAttribution: droplet commented

doesn't it:

git apply --index test.patch
yarn build:js
git diff 
(has diff, then it's wrong)

then meant:

git apply --index test.patch
// then run this: https://www.drupal.org/node/2879072
git diff
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

@droplet the point is to have a command so that scripts can check without actually changing the code.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
2-5-interdiff.txt4 KB
2880013-5.patch4.98 KB

Let's do the transpiling the same piece of code to make it easier to maintain.

droplet’s picture

droplet CreditAttribution: droplet commented
FileSize
2880013-6.patch4.28 KB
interdiff.patch3.34 KB

Hmm.. Or this way:

yarn run build:js

yarn run build:js -- --file misc/drupal.es6.js
yarn run build:js -- --file misc/drupal.es6.js --file misc/drupal.init.es6.js

yarn run build:js -- --check
yarn run build:js -- --check --file misc/drupal.es6.js

No Yarn.lock updates, run yarn why minimist to get more info :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
6-7-interdiff.txt5.14 KB
2880013-2-7.patch5.67 KB

I still think it is work separating the writing and checking concerns and also only doing the argument parsing in one place. Helps mitigate side-effects. Patch attached keeps the --check flag but moves all the argument handling to babel-es-build.js.

The last submitted patch, 6: interdiff.patch, failed testing.

droplet’s picture

droplet CreditAttribution: droplet commented
FileSize
2880013-8.patch5.95 KB
interdiff.patch1.61 KB
c20170521_184113.png20.6 KB

@alexpott that works.

Also, I think we want to collect all useful info rather than throw error to stop script immediately


(highlighted box is after patched)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

@droplet thanks for making that change!

This is looking really good now... I've confirmed that the process exits with an error when there is one. Like it.

Status: Needs review » Needs work

The last submitted patch, 9: interdiff.patch, failed testing.

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented 
+++ b/core/scripts/js/babel-es6-build.js
@@ -1,7 +1,14 @@
+ * npm run build:js -- --file misc/drupal.es6.js --file misc/drupal.init.es6.js

Should say yarn run build:js

Just a minor nit about the docs. Other than that I like this and the separation of the code into smaller, easier to test parts, makes me happy.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs work » Needs review
FileSize
9-13-interdiff.txt883 bytes
2880013-2-13.patch6.03 KB

Updated the patch for #12

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented
Status: Needs review » Reviewed & tested by the community

@alexpott

Great. I like this and it's probably important to get this in before we start working on actual improvements to core JavaScript.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.4.x, thanks!

  • catch committed bc12e67 on 8.4.x 
    Issue #2880013 by alexpott, droplet: Add command to check if .es6.js has...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

This just helped me find #2880560: Fix comment added to top of .js files by build process

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.