#credit @drpal also
git commit -m 'Issue #2910705 by droplet, drpal: Update JS Build Script Packages'

Update versions of JS Build Script dependencies.

CommentFileSizeAuthor
#17 interdiff-2910705-16-17.txt70.12 KBGrandmaGlassesRopeMan
#17 2910705-17.patch109.63 KBGrandmaGlassesRopeMan
#16 2910705-16.patch53.55 KBdroplet
#4 2910705-4.patch20.79 KBdroplet
upgrade-js-build-script-packages.patch20.79 KBdroplet

Comments

droplet created an issue. See original summary.

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
commented

@droplet I had created #2895713: Update Babel and babel-preset-env to their latest versions which should probably just be updated with this information.

droplet’s picture

droplet commented
Issue summary: View changes
droplet’s picture

droplet commented
Priority: Normal » Major
StatusFileSize
new 2910705-4.patch20.79 KB

The build script has a compatible error with node 8.8.0+

GrandmaGlassesRopeMan’s picture

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

Ok. This looks good. I tested this locally and didn't see any build-time changes. 👍

xjm’s picture

xjm
she/her
Primary language English
commented

Updating issue credit.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

Thanks for the patch! I think this should probably go into 8.5.x only.

+++ b/core/yarn.lock
@@ -847,9 +950,9 @@ cosmiconfig@^2.1.1:
-cross-env@^4.0.0:
-  version "4.0.0"
-  resolved "https://registry.yarnpkg.com/cross-env/-/cross-env-4.0.0.tgz#16083862d08275a4628b0b243b121bedaa55dd80"
+cross-env@^5.1.0:
+  version "5.1.0"
+  resolved "https://registry.yarnpkg.com/cross-env/-/cross-env-5.1.0.tgz#1f12d6b3777d5847dcf9cf39fbee3c6a76dd5058"

This one looks like a major update; can we check and confirm what BC breaks if any this package might have?

It's probably okay but we should ask the question each time.

GrandmaGlassesRopeMan’s picture

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

There's just, Scripts using quotes or escape sequences will see a difference in behavior., which was noted in the 5.0.0 release notes. We aren't doing that so it should not be an issue.

droplet’s picture

droplet commented

Technically, there are no BC breaks in NodeJS packages we have to check.

For non-CORE scripts, they should always specify the version in their own package.json rather than include the ref packages from another package. And you need to call the script command in the CORE package rather than modify it directly. It's independent.

It reminds me to check the scripts in "core\scripts\js", I, myself do not expect anyone to include these files into their own scripts.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record, +8.5.0 release notes

Thanks @drpal and @droplet.

Let's at least add a CR about this issue that covers #8 and maybe also reminds site owners to check this and maybe includes some reminder about #9. Once we have that, I think this is okay for a minor release.

Probably we should also mention it 8.5.x release notes (with a link to the CR) in case someone is doing something strange.

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
commented
Status: Needs work » Needs review

- cr => https://www.drupal.org/node/2925008

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Whoops, looks like this missed getting its needed review. I made some small edits to the CR; looks good.

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
commented

@xjm Since this sat at needs review for so long some of the updates to the dependencies are probably out of date.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

ah, Can we update it then? I imagine this would be something to get in before beta. What would the consequences be of being on the old packages throughout 8.5?

droplet’s picture

droplet commented
StatusFileSize
new 2910705-16.patch53.55 KB

I think we should add "^" (ref) before all packages

Then to run:

// use a more up-to-date version, less buggy
npm install -g yarn

// for minor Core release
yarn upgrade-interactive

// for major Core release
yarn upgrade-interactive --latest
GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
commented
StatusFileSize
new 2910705-17.patch109.63 KB
new interdiff-2910705-16-17.txt70.12 KB

- set caret ranges on all packages
- update versions of other things that have changed

justafish’s picture

justafish
she/her
Primary language English
Location London, UK
commented
Status: Needs review » Reviewed & tested by the community

👍

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 17: 2910705-17.patch, failed testing. View results

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
commented

Alright, a few strange things are happening,

- the php yaml parser doesn't like this config file,
- why are we testing yaml files within the node_modules directory? those are all javascript dependencies?

GrandmaGlassesRopeMan’s picture

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

  • lauriii committed 9788b22 on 8.6.x 
    Issue #2910705 by droplet, drpal: Update JS Build Script Packages

  • lauriii committed 4984a6d on 8.5.x 
    Issue #2910705 by droplet, drpal: Update JS Build Script Packages
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs change record

Tested manually the build tools and checked for any unexpected changes in the compiled JS. Everything looked normal since there was no changes in the compiled JS at all.

The major release that @xjm had already pointed on #7 also caught my eye. I checked the release notes to make sure we are not affected. I came to same conclusion as @drpal did on #8.

Committed to 8.6.x and cherry-picked to 8.5.x. Thanks!

Status: Fixed » Closed (fixed)

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