Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
At some point some issues were introduced in the package.json script.
- 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.
- 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.
Comment | File | Size | Author |
---|---|---|---|
#26 | package-update.patch | 958 bytes | droplet |
#22 | 2880004-22.patch | 8.58 KB | GrandmaGlassesRopeMan |
#21 | interdiff-2880004-18-21.txt | 2.36 KB | GrandmaGlassesRopeMan |
#21 | 2880004-21.patch | 8.65 KB | GrandmaGlassesRopeMan |
#18 | core-es6-scripts-2880004-18.patch | 8.63 KB | nod_ |
Comments
Comment #2
nod_Comment #3
droplet CreditAttribution: droplet commentedI 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.
Comment #4
GrandmaGlassesRopeManSomeone might have another version of
eslint
in their path. I think this should be./node_modules/bin/eslint --ext=.es6.js . || exit 0
Comment #5
nod_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.
Comment #6
GrandmaGlassesRopeMan@nod_
Oh neat, I guess that's changed. Awesome.
Comment #7
droplet CreditAttribution: droplet commented@drpal point is valid because it's not everyone run
yarn install
locally.Comment #8
nod_Should be node_modules/.bin/eslint then
Comment #9
GrandmaGlassesRopeMan@nod_ @droplet
Comment #10
droplet CreditAttribution: droplet commentedGo Go Go!
Comment #11
nod_Just adding a build:js-dev command similar to watch:js-dev. Otherwise no simple way to have sourcemap on all js files.
Comment #12
alexpottI'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
Comment #13
GrandmaGlassesRopeManThe file in
node_modules/.bin
is just a symlink tonode_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.Comment #14
GrandmaGlassesRopeManComment #15
alexpottBut didn't #5 point out that this will use the local version over the global version. It certainly does for me.
Comment #16
droplet CreditAttribution: droplet commented@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 :)
Comment #17
alexpott@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.
Comment #18
nod_Added a little something to help reroll patches, that is on topic :p
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.
Comment #19
alexpottI 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.
We should just be leaving this line alone.
Comment #20
droplet CreditAttribution: droplet commentedInteresting 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)
Comment #21
GrandmaGlassesRopeManAn attempt to get this back on track. This reverts the repath of
lint:core-js
,and sets the dependencies to be exact versionsthis 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.Comment #22
GrandmaGlassesRopeManAlright, this reverts all the cart version stuff with Yarn, gets the patch to apply again and keeps the path of
eslint
fromlint:core-js
.Comment #23
alexpottThe concerns in #19.1 haven't really been addressed.
Comment #24
nod_Yeah get rid of the patch-js thing
Comment #25
droplet CreditAttribution: droplet commentedJust 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)
Comment #26
droplet CreditAttribution: droplet commentedIt's the beginning. Let's push it as fast as we can before the message spreading over the world :)
Comment #27
GrandmaGlassesRopeMan@droplet
I'm not sure this is needed. We should never be building all the JS with source maps.
Comment #28
droplet CreditAttribution: droplet commentedFor some reason, you may want the sourceMap to debug without modifying the files (in watchMode).
Comment #29
droplet CreditAttribution: droplet commentedI still prefer to recommend anyone use `build:js` and `watch:js`
Comment #30
GrandmaGlassesRopeMan@droplet
Alright, I can see that. :)
Comment #31
droplet CreditAttribution: droplet commentedI 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.
Comment #32
GrandmaGlassesRopeMan@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.Comment #34
catchCommitted 4c03c96 and pushed to 8.4.x. Thanks!