Skip to content
This repository has been archived by the owner on Feb 29, 2020. It is now read-only.

Bug 1597301 - Greening up dev deps #5384

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Bug 1597301 - Greening up dev deps #5384

wants to merge 1 commit into from

Conversation

gvn
Copy link
Contributor

@gvn gvn commented Nov 20, 2019

next-update --test './node_modules/.bin/npm-run-all test buildmc' --type dev

1 failed upgrade: karma-sinon 1.0.5 -> 1.8.0

Ran on OSX via:

node @ v8.16.2
npm @ 6.9.2

@gvn gvn requested review from piatra and dmose November 20, 2019 23:34
@gvn
Copy link
Contributor Author

gvn commented Nov 20, 2019

Looks like Prettier update blew up, perhaps due to new rules despite being a minor bump? Should I separate that out into its own PR with files prettified to pass?

@piatra
Copy link
Contributor

piatra commented Nov 25, 2019

Looks like Prettier update blew up, perhaps due to new rules despite being a minor bump? Should I separate that out into its own PR with files prettified to pass?

Yes I think it would be best to split it out if it involves code changes.
On second thought we might want to confirm/compare with the m-c version. Are we at risk of exporting something not compatible m-c? In that case we should hold off on the upgrade.

Curious why karma-sinon failed, is that easily fixable as well?

@gvn our package.json specifies npm 6.9. I don't think it's a big difference in the end result but it would mean that everyone else doing npm install would have a different package-lock.json.

@gvn
Copy link
Contributor Author

gvn commented Nov 25, 2019

@gvn our package.json specifies npm 6.9. I don't think it's a big difference in the end result but it would mean that everyone else doing npm install would have a different package-lock.json.

I accidentally put the wrong nvm profile in when I was writing that comment. Correct versions are now listed! (npm was/is at 6.9.2)

@piatra
Copy link
Contributor

piatra commented Nov 26, 2019

Hm strage
https://www.npmjs.com/package/karma-sinon
There is no 1.8.0 release.

"enzyme": "3.10.0",
"enzyme-adapter-react-16": "1.15.1",
"eslint": "6.6.0",
"eslint-config-prettier": "6.7.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Prettier is safe to update but we should keep the configs in sync with m-c.

"eslint-plugin-prettier": "3.0.1",
"eslint-plugin-react": "7.13.0",
"eslint-plugin-react-hooks": "1.6.0",
"eslint-plugin-prettier": "3.1.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

This is ok too.

@dmose
Copy link
Member

dmose commented Dec 10, 2019

Sorry for the long delay getting to this!

@gvn, I had not seen next-update before, that's very cool!

Prettier makes changes with updated versions (see https://prettier.io/docs/en/install.html). This means that we need to be using the same version as mozilla-central, so it can't be upgraded. We should probably add a package.json comment to that effect to avoid future confusion:

    "//": "Prettier must be the same version as the one in mozilla-central's top-level package.json",
    "prettier": "1.17.0"

I suppose we could add (meta-data) [https://github.com/bahmutov/next-update#ignoring-or-skipping-some-modules] to make next-update do the right thing also.

Copy link
Member

@dmose dmose left a comment

Choose a reason for hiding this comment

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

Please update to leave prettier unchanged (and adding the comments/meta-data suggested below seems worthwhile).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants