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

Update generated app development package.json dependencies #553

Merged
merged 5 commits into from
Oct 23, 2020
Merged

Update generated app development package.json dependencies #553

merged 5 commits into from
Oct 23, 2020

Conversation

stephendolan
Copy link
Member

@stephendolan stephendolan commented Sep 17, 2020

This PR aims to generate new Lucky applications with up-to-date and secure versions of development dependencies.

Before I get into details, I've been running these dependencies in Lucky Jumpstart Lucky apps without issue for quite a while now. I also want to re-emphasize that this only bumps development dependencies, since all production dependencies are up to date.

The long term discussion about how and if we keep this up to date is here.

Below, I'll provide a rundown of the version changes made, as well as a TL;DR list of breaking changes that were noted.

This PR tackles:

  • Alphabetizing the dependencies: section, since that seems to be the intention
    • Update - Instead of alphabetizing, moved @babel/compat-data to devDependencies
  • Bumping compression-webpack-plugin from ^3.0.0 to ^6.0.1 (Changelog here)
    • 3 -> 4 breaking changes
    • 4 -> 5 breaking changes
      • default value of the filename option is '[path].gz'
      • use processAssets hook for webpack@5 compatibility, it can create incompatibility with plugins that do not support webpack@5, please open an issue in their repositories
    • 5 -> 6 breaking changes
  • Bumping laravel-mix from ^4.0.0 to ^5.0.5 (Changelog here)
    • 4 -> 5 breaking changes
      • Use sass-loader 8
  • Bumping resolve-url-loader from 2.3.1 to ^3.1.1 (Changelog here)
    • 2 -> 3 breaking changes
      • Multiple options changed or deprecated.
      • Removed file search "magic" in favour of join option.
      • Errors always fail and are no longer swallowed.
      • Processing absolute asset paths requires root option to be set.
  • Bumping sass from 1.17.1 to ^1.26.10 (Changelog here)
  • Bumping sass-loader from ^7.3.1 to ^10.0.2 (Changelog here)
    • 7 -> 8 breaking changes
      • minimum required webpack version is 4.36.0
      • minimum required node.js version is 8.9.0
      • move all sass (includePaths, importer, functions, outputStyle) options to the sassOptions option. The functions option can't be used as Function, you should use sassOption as Function to achieve this.
      • the data option was renamed to the prependData option
      • default value of the sourceMap option depends on the devtool value (eval/false values don't enable source map generation)
    • 8 -> 9 breaking changes
      • minimum supported Nodejs version is 10.13
      • prefer sass (dart-sass) by default, it is strongly recommended to migrate on sass (dart-sass)
      • the prependData option was removed in favor the additionalData option, see docs
      • when the sourceMap is true, sassOptions.sourceMap, sassOptions.sourceMapContents, sassOptions.sourceMapEmbed, sassOptions.sourceMapRoot and sassOptions.omitSourceMapUrl will be ignored.
    • 9 -> 10 breaking changes
      • loader generates absolute sources in source maps, also avoids modifying sass source maps if the sourceMap option is false
  • Bumping vue-template-compiler from ^2.5.22 to ^2.6.12 (Changelog here)
    • Update - Removed this instead

@jwoertink
Copy link
Member

Wow! Great detail.

Since you've gone deep in to this already, what is your assessment on the use of each of these? Like vue-template-compiler; do we even need this? I'm not sure what we use it for. Also, with this update to mix, is there anything in the generated webpack.mix.js file that needs to be updated? I think if there's any assets that aren't needed for a "hello world" lucky app, maybe we look in to ditching them. Also the @babel/compat-data I'm wondering if that should be a dev dependency. I don't really know what that does either, but I'm under the impression that anything babel related is for development since webpack will compile things in to your cross-browser compatible chunks.

@stephendolan
Copy link
Member Author

From what I've looked into so far, I actually can't answer any of those questions... maybe @paulcsmith can give some background, but in the meantime, I'll play with:

  • Generating the "default" laravel-mix file (if that exists) and checking against our default
  • Looking into why babel is in production dependencies
  • Figuring out what vue-template-compiler is used for

…ecure versions of development dependencies.

Before I get into details, I've been running these dependencies in [Lucky Jumpstart](https://github.com/stephendolan/lucky_jumpstart) Lucky apps without issue for quite a while now. I also want to re-emphasize that this only bumps **development** dependencies, since all production dependencies are up to date.

The long term discussion about how and if we keep this up to date is [here](luckyframework/lucky#1261).

Below, I'll provide a rundown of the version changes made, as well as a TL;DR list of breaking changes that were noted.

This PR tackles:

- Alphabetizing the `dependencies:` section, since that seems to be the intention
- Bumping `compression-webpack-plugin` from `^3.0.0` to `^6.0.1` ([Changelog here](https://github.com/webpack-contrib/compression-webpack-plugin/blob/master/CHANGELOG.md))
  - 3 -> 4 breaking changes
    - the cache is true by default for webpack@4
    - the cache option is ignored in webpack 5. Please use https://webpack.js.org/configuration/other-options/#cache.
    - minimum supported Node.js version is 10.13
  - 4 -> 5 breaking changes
    - default value of the filename option is '[path].gz'
    - use processAssets hook for webpack@5 compatibility, it can create incompatibility with plugins that do not support webpack@5, please open an issue in their repositories
  - 5 -> 6 breaking changes
    - default value of the filename option was changed to "[path][base].gz"
    - removed the [dir] placeholder, please use the [path] placeholder
    - the Function type of the filename option should return value with placeholders, please see an example: https://github.com/webpack-contrib/compression-webpack-plugin#function-1
- Bumping `laravel-mix` from `^4.0.0` to `^5.0.5` ([Changelog here](https://github.com/JeffreyWay/laravel-mix/releases))
  - 4 -> 5 breaking changes
    - Use `sass-loader` `8`
- Bumping `resolve-url-loader` from `2.3.1` to `^3.1.1` ([Changelog here](https://github.com/bholloway/resolve-url-loader/releases))
  - 2 -> 3 breaking changes
    - Multiple options changed or deprecated.
    - Removed file search "magic" in favour of join option.
    - Errors always fail and are no longer swallowed.
    - Processing absolute asset paths requires root option to be set.
- Bumping `sass` from `1.17.1` to `^1.26.10` ([Changelog here](https://github.com/sass/dart-sass/blob/master/CHANGELOG.md))
- Bumping `sass-loader` from `^7.3.1` to `^10.0.2` ([Changelog here](https://github.com/webpack-contrib/sass-loader/blob/master/CHANGELOG.md))
  - 7 -> 8 breaking changes
    - minimum required webpack version is 4.36.0
    - minimum required node.js version is 8.9.0
    - move all sass (includePaths, importer, functions, outputStyle) options to the sassOptions option. The functions option can't be used as Function, you should use sassOption as Function to achieve this.
    - the data option was renamed to the prependData option
    - default value of the sourceMap option depends on the devtool value (eval/false values don't enable source map generation)
  - 8 -> 9 breaking changes
    - minimum supported Nodejs version is 10.13
    - prefer sass (dart-sass) by default, it is strongly recommended to migrate on sass (dart-sass)
    - the prependData option was removed in favor the additionalData option, see docs
    - when the sourceMap is true, sassOptions.sourceMap, sassOptions.sourceMapContents, sassOptions.sourceMapEmbed, sassOptions.sourceMapRoot and sassOptions.omitSourceMapUrl will be ignored.
  - 9 -> 10 breaking changes
    - loader generates absolute sources in source maps, also avoids modifying sass source maps if the sourceMap option is false
- Bumping `vue-template-compiler` from `^2.5.22` to `^2.6.12` ([Changelog here](https://github.com/vuejs/vue/tree/dev/packages/vue-template-compiler#readme))
@stephendolan
Copy link
Member Author

stephendolan commented Sep 21, 2020

@jwoertink I come bearing (good?) news!

Laravel Mix
Our current config file actually looks just fine! We have a bunch of content, but most of it consists of commented-out options that are helpful for customization.

That said, I can't find the source that (I think) Paul must have copied from to get that sample file, so I don't have something explicit to diff against.

Babel in Dev
Here's where it was added, and the same question was asked: #517
That said, I was able to move it to a dev dependency now and both local dev and yarn prod work without errors/warnings.
I was also able to deploy Lucky Jumpstart to Heroku with the change, and it loads successfully: https://lucky-jumpstart.herokuapp.com/

Vue Template Compiler
It looks like this was added here in the Laravel Mix 4 upgrade. I can't find a reason immediately upon searching through the upgrade notes or anything.
I removed this and am getting no warnings, though I did notice that it's actually automatically installed with this:

  $ yarn run webpack --progress --hide-modules --color --config=node_modules/laravel-mix/setup/webpack.config.js
  $ /Users/stephen/Repos/@oss/lucky_cli/test-project/node_modules/.bin/webpack --progress --hide-modules --color --config=node_modules/laravel-mix/setup/webpack.config.js
   	Additional dependencies must be installed. This will only take a moment.

   	Running: npm install vue-template-compiler --save-dev --production=false

In summary:

  • I left the webpack.mix.js file alone
  • I moved @babel/compat-data to devDependencies
  • I removed vue-template-compiler
  • I modified our .gitignore slightly to make it more robust against working with these items in a dev capacity

@stephendolan
Copy link
Member Author

Not sure if this could be a cache issue... but I have no idea why this'd be running:
https://github.com/luckyframework/lucky_cli/pull/553/checks?check_run_id=1144104086#step:13:99

@jwoertink
Copy link
Member

Ah weird. Do we need a main key in the package.json now? If build fails, we can always move that stuff back to how it was. I'll take a look later too to see what I can find out.

@stephendolan
Copy link
Member Author

Yeah, super strange because it doesn't occur when I run ./script/test locally either

@paulcsmith
Copy link
Member

The Laravel mix files comes from https://laravel-mix.com/docs/5.0/installation#stand-alone-project so that could work as a diff. I don't have time to test this at the moment, but I am glad you are updating everything. Thank you!

@stephendolan
Copy link
Member Author

stephendolan commented Oct 23, 2020

Found out why we have vue-template-compiler. It's a relic that's being removed in Laravel Mix v6, which should be out soon.

laravel-mix/laravel-mix#2339

If I can get CI/CD passing, I'll be sure to watch that repo and create a new one to migrate us to 6 later.

@stephendolan
Copy link
Member Author

Victory! @jwoertink if you don't mind giving this a quick review/approval I'll get it merged, followed by the Dependabot updates.

Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

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

Sweet. It'll be nice to keep these up to date.

@@ -14,12 +13,13 @@
"prod": "NODE_ENV=production yarn run webpack --progress --hide-modules --color --config=node_modules/laravel-mix/setup/webpack.config.js"
},
"devDependencies": {
"@babel/compat-data": "^7.9.0",
Copy link
Member

Choose a reason for hiding this comment

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

Nice 👍

/tmp/

# Libraries don't need dependency lock
# Dependencies will be locked in application that uses them
/shard.lock
yarn.lock
Copy link
Member

Choose a reason for hiding this comment

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

Should this also be added to generated projects? https://github.com/luckyframework/lucky_cli/blob/master/src/generators/web.cr#L75 I've never really known what best practice for that sort of deal is.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've always seen yarn.lock committed to repos, so I think it's probably safer to leave it as is!

This was mostly to ensure that folks playing with these deps in the future don't accidentally push a yarn.lock, which could have some negative side effects like os-specific package sub-dependencies (if my limited knowledge serves me)

@stephendolan stephendolan merged commit f0ae36c into luckyframework:master Oct 23, 2020
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.

None yet

3 participants