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 yarn.lock #5811

Merged
merged 2 commits into from
Jan 31, 2020
Merged

Update yarn.lock #5811

merged 2 commits into from
Jan 31, 2020

Conversation

rhymes
Copy link
Contributor

@rhymes rhymes commented Jan 29, 2020

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

I ran yarn install --dev on master as I often do on each breanch when switching for PR reviews and I noticed this time it updated the lock file, which makes me suspect it wasn't updated correctly in a previous PR.

Try to do the same on your local master and see if the yarn.lock gets updated locally

@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Jan 29, 2020
@citizen428
Copy link
Contributor

I noticed this yesterday too, but then forgot about it as I was deep in my (backend) PR and config.webpacker.check_yarn_integrity = true in development.rb was a good enough temp fix for me.

But I agree, we should probably update this file, as it seems it wasn't checked in last time packages were updated.

@nickytonline
Copy link
Contributor

I had changes to yarn.lock the other day in regards to fixing Storybook (#5783), but made sure it updated correctly. Just ensure with these changes that you can still view Storybook (npm run storybook, then navigate to http://localhost:6006)

@rhymes
Copy link
Contributor Author

rhymes commented Jan 30, 2020

@citizen428 I actually thought it was always true in development! @nickytonline, is there a reason to keep the yarn integrity check disabled in development?

@nickytonline:

Just ensure with these changes that you can still view Storybook (npm run storybook, then navigate to http://localhost:6006

You mean yarn storybook ;-) ? Yeah, it works:

➜  devto git:(rhymes/update-yarn-lockfile) yarn storybook
yarn run v1.21.1
$ start-storybook -p 6006 -c app/javascript/.storybook -s app/javascript/.storybook/assets
info @storybook/react v3.4.11
info
info => Loading static files from: /Users/rhymes/Development/devto/app/javascript/.storybook/assets .
info => Loading custom .babelrc
info => Loading custom addons config.
info => Loading custom webpack config (full-control mode).
webpack built 1f85e6ae6f8d02aec3e9 in 22575ms
info Storybook started on => http://localhost:6006/
info

Screenshot 2020-01-30 at 11 51 18 AM

@nickytonline
Copy link
Contributor

nickytonline commented Jan 30, 2020

@nickytonline, is there a reason to keep the yarn integrity check disabled in development?

@rhymes, I have more experience with npm over yarn, but to me it makes sense to have it enabled all the time.

@rhymes
Copy link
Contributor Author

rhymes commented Jan 30, 2020

I traced the optional check for integrity to this PR:

#296

see this discussion #296 (review)

Basically we can't enable it by default, because it's broken under docker: rails/webpacker#1568

I'm going to add a note in development.rb about it

@rhymes rhymes requested a review from a team January 30, 2020 14:31
# As the integrity check is currently broken under Docker with webpacker,
# we can't enable this flag by default
# see <https://github.com/thepracticaldev/dev.to/pull/296#discussion_r210635685>
config.webpacker.check_yarn_integrity = ENV.fetch("YARN_INTEGRITY_ENABLED", "true") == "true"
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 for inlining this.

@pr-triage pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Jan 31, 2020
Copy link
Contributor

@nickytonline nickytonline left a comment

Choose a reason for hiding this comment

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

:shipit:

@mstruve mstruve merged commit c64a7c5 into forem:master Jan 31, 2020
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Jan 31, 2020
@rhymes rhymes deleted the rhymes/update-yarn-lockfile branch February 1, 2020 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: merged bot applied label for PR's that are merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants