At some point some issues were introduced in the package.json script.

  1. eslint is run with the --fix parameter, because the testbot should be using it it's a bad thing to make this command change files.
  2. watch-js is useless while core-patching. I want to run watch-js and send that to the patch-reviewing-machine, don't make me run build-js.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nod_ created an issue. See original summary.

nod_’s picture

droplet’s picture

I love it if you asked me. I seldom use sourcemap during debugging (in the Drupal JS). LOL. It saved one step to rebuilding JS after watcher.

GrandmaGlassesRopeMan’s picture

Status: Needs review » Needs work
+++ b/core/package.json
@@ -5,8 +5,9 @@
+    "lint:core-js": "eslint --ext=.es6.js . || exit 0",

Someone might have another version of eslint in their path. I think this should be ./node_modules/bin/eslint --ext=.es6.js . || exit 0

nod_’s picture

Status: Needs work » Needs review

nope, local will be used in priority see doc: https://docs.npmjs.com/cli/run-script

also styllint and cross-env are used without the prefix.

GrandmaGlassesRopeMan’s picture

Status: Needs review » Reviewed & tested by the community

@nod_

Oh neat, I guess that's changed. Awesome.

droplet’s picture

@drpal point is valid because it's not everyone run yarn install locally.

nod_’s picture

Should be node_modules/.bin/eslint then

GrandmaGlassesRopeMan’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
648 bytes
851 bytes

@nod_ @droplet

droplet’s picture

Status: Needs review » Reviewed & tested by the community

Go Go Go!

nod_’s picture

FileSize
944 bytes

Just adding a build:js-dev command similar to watch:js-dev. Otherwise no simple way to have sourcemap on all js files.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I'm not convinced by #7. I don't think we should be changing the path to eslint in this issue. Firstly, it is out-of-scope and secondly, I don't think we support running these commands without

everyone run yarn install locally.
GrandmaGlassesRopeMan’s picture

+++ b/core/package.json
@@ -5,8 +5,10 @@
+    "lint:core-js": "./node_modules/.bin/eslint --ext=.es6.js . || exit 0",

The file in node_modules/.bin is just a symlink to node_modules/eslint/bin/eslint.js. What we're doing here is preventing people from, who at some point in the past, have installed eslint globally, available in their path, and then don't understand why they can't lint using the new JavaScript standards.

GrandmaGlassesRopeMan’s picture

Status: Needs work » Needs review
alexpott’s picture

But didn't #5 point out that this will use the local version over the global version. It certainly does for me.

droplet’s picture

@alexpott,

It's a way to restrict our developer to run build tools locally with code rather than documents or educating them on each issue :)

alexpott’s picture

Status: Needs review » Needs work

@droplet it is still out of scope for this issue then - and we should open another issue to use paths to binaries in package.json and do both eslint and stylelint at the same time and maybe a comment to the file about why so if some adds future commands they do the same. This issue should be about adding the -dev commands.

nod_’s picture

Status: Needs work » Needs review
FileSize
8.63 KB

Added a little something to help reroll patches, that is on topic :p

 yarn run patch:js -- https://www.drupal.org/files/issues/2725255-32.patch

Essentially it renames js files from .es6.js to .js so that the patch applies and then renames it back to .es6.js and compile it.

alexpott’s picture

  1. +++ b/core/package.json
    @@ -4,9 +4,12 @@
    +    "patch:js": "node ./scripts/js/patch-es6.js",
    

    I like the functionality. I would call it something to do with es5 rerolling. Because once the patch is updated for es6 then this is not needed. In fact, that gives me pause because this is only going to have a very very limited shelf life - so personally I'd still do this in a separate issue.

    An issue just to add build:js-dev and watch:js-dev would get done super quick.

  2. +++ b/core/package.json
    @@ -4,9 +4,12 @@
    -    "lint:core-js": "node ./node_modules/eslint/bin/eslint.js --ext=.es6.js . --fix || exit 0",
    ...
    +    "lint:core-js": "eslint --ext=.es6.js . || exit 0",
    

    We should just be leaving this line alone.

droplet’s picture

 yarn run patch:js -- https://www.drupal.org/files/issues/2725255-32.patch

Interesting idea. But only useful before the ESLint autofix patch.

Also, I preferred lightweight package.json (node_modules dir). I'm fine to do 2 steps but remove node-fetch..etc (And find a similar package from current node_modules instead)

GrandmaGlassesRopeMan’s picture

An attempt to get this back on track. This reverts the repath of lint:core-js, and sets the dependencies to be exact versions this doesn't matter because Yarn is doing the correct thing, confusingly.

About dependencies, if we want to prefix them all with a ^, let's open a followup for that and set everything to one standard.

GrandmaGlassesRopeMan’s picture

Alright, this reverts all the cart version stuff with Yarn, gets the patch to apply again and keeps the path of eslint from lint:core-js.

alexpott’s picture

The concerns in #19.1 haven't really been addressed.

nod_’s picture

Yeah get rid of the patch-js thing

droplet’s picture

Just a note:

I tested PATCH-JS and it doesn't work: #1988968: Drupal.ajax does not guarantee that "add new JS file to page" commands have finished before calling said JS

I have a simple idea:

1. git apply --stat (safer than regex in patch directly)
2. regex .js only
3. string replace in patch
4. DONE :)

(and stream buffer than tmp file)

droplet’s picture

It's the beginning. Let's push it as fast as we can before the message spreading over the world :)

GrandmaGlassesRopeMan’s picture

Status: Needs review » Needs work

@droplet

+++ b/core/package.json
@@ -5,8 +5,10 @@
+    "build:js-dev": "cross-env NODE_ENV=development node ./scripts/js/babel-es6-build.js",

I'm not sure this is needed. We should never be building all the JS with source maps.

droplet’s picture

Status: Needs work » Needs review

For some reason, you may want the sourceMap to debug without modifying the files (in watchMode).

droplet’s picture

I still prefer to recommend anyone use `build:js` and `watch:js`

GrandmaGlassesRopeMan’s picture

Status: Needs review » Reviewed & tested by the community

@droplet

Alright, I can see that. :)

droplet’s picture

I thought, at some point, we should just keep a single entry point.

drupal.js --watch --check --source-map --file=misc/ajax.js

for the future, we may support
--babel-env=chrome
we want to pass babel-env config to build a script for my mobile only site. We need not support IEs / Edge.

--min
We won't serve it for GIT, patching conflict or other reason. But I, as a site owner, I preferred uglifyjs it.

We should always provide some more way for our advanced users. It's 2017. Don't let PHP build process blocking us anymore.

GrandmaGlassesRopeMan’s picture

@droplet

Exactly. I'd like to see us refactor these tools to provide a single entry point. Having a --min would be wonderful if we actually get to the point of having a real build process with the packager.

  • catch committed 4c03c96 on 8.4.x
    Issue #2880004 by drpal, nod_, droplet, alexpott: Improve (again) ES6...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed 4c03c96 and pushed to 8.4.x. Thanks!

Status: Fixed » Closed (fixed)

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