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

Bump deps and use npm7 #319

Merged
merged 2 commits into from
Jun 15, 2021
Merged

Bump deps and use npm7 #319

merged 2 commits into from
Jun 15, 2021

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Jun 11, 2021

Please test on your respective apps and see what's breaking ⚠️
(list built automatically, feel free to remove unrelated entries)

  1. Remove all devDeps like babel, eslint, webpack, loaders... etc
  2. Delete your lockfile
  3. Make sure you use npm 7 ⚠️
  4. npm i -D @nextcloud/webpack-vue-config@latest @nextcloud/eslint-config@latest @nextcloud/babel-config@latest @nextcloud/browserslist-config@latest @nextcloud/stylelint-config@latest
  5. Check your stylelint config https://github.com/nextcloud/stylelint-config#usage
  6. Check your babel config https://github.com/nextcloud/babel-config#usage
  7. Check your eslint config https://github.com/nextcloud/eslint-config#usage
  8. Build and check for errors
  9. Profit

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv skjnldsv force-pushed the feat/deps-npm7 branch 2 times, most recently from 03c075f to ca9b76d Compare June 11, 2021 09:54
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv skjnldsv self-assigned this Jun 11, 2021
@skjnldsv skjnldsv added 3. to review Waiting for reviews dependencies Pull requests that update a dependency file enhancement labels Jun 11, 2021
Copy link
Member

@raimund-schluessler raimund-schluessler left a comment

Choose a reason for hiding this comment

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

Configuration works fine for Tasks.

Copy link
Member

@korelstar korelstar left a comment

Choose a reason for hiding this comment

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

🎉
Works for notes, see nextcloud/notes#727

Copy link
Member

@jotoeri jotoeri left a comment

Choose a reason for hiding this comment

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

@korelstar
Copy link
Member

Just for information: if you use node v15, you do not need to explicitly install npm v7, since it's already included. This simplifies the GitHub workflow a little bit.

@skjnldsv
Copy link
Member Author

Just for information: if you use node v15, you do not need to explicitly install npm v7, since it's already included. This simplifies the GitHub workflow a little bit.

Node lts is still 14 :)
https://nodejs.org/en/about/releases/

@skjnldsv
Copy link
Member Author

skjnldsv commented Jun 14, 2021

One issue I encountered!
If your vue is set to a different version than the vue-template-compiler of the @nextcloud/webpack-vue-config, it will throw (well, I guess you will receive the update or will definitely see the issue). That's the only issue I found so far. But that's an issue with the whole process, it can already happen

@nickvergessen
Copy link
Member

announcementcenter works: nextcloud/announcementcenter#360

@nickvergessen
Copy link
Member

user retention works: nextcloud/user_retention#361

@raimund-schluessler
Copy link
Member

@skjnldsv I think nextcloud-vue is does not have a PR for npm7 yet, right (just asking because it's checked in the list)?

@skjnldsv
Copy link
Member Author

I think nextcloud-vue is does not have a PR for npm7 yet, right (just asking because it's checked in the list)?

Ah sorry, I thought nextcloud-libraries/nextcloud-vue#2037 was it, indeed! 👍
Wanna do it there?

@raimund-schluessler
Copy link
Member

I think nextcloud-vue is does not have a PR for npm7 yet, right (just asking because it's checked in the list)?

Ah sorry, I thought nextcloud/nextcloud-vue#2037 was it, indeed! 👍
Wanna do it there?

Yes, I was about to try that yesterday already, but thought it would be cleaner to first merge the common webpack config 😃

@nickvergessen
Copy link
Member

Registration works: nextcloud/registration#309

@skjnldsv
Copy link
Member Author

Ok, let's go then, the process seems to be going well?

@skjnldsv skjnldsv merged commit ecdae87 into master Jun 15, 2021
@skjnldsv skjnldsv deleted the feat/deps-npm7 branch June 15, 2021 13:33
@skjnldsv
Copy link
Member Author

skjnldsv commented Jun 16, 2021

@korelstar @raimund-schluessler @jotoeri @juliushaertl @artonge @eneiluj @ChristophWurst
You can adjust the package.json engine to also specify which npm version is needed:

	"engines": {
		"node": ">=14.0.0",
		"npm": ">=7.0.0"
	},

@mejo-
Copy link
Member

mejo- commented Jun 25, 2021

Collectives app also migrated now. Thanks a lot for the detailed instructions @skjnldsv!

Since in our case some extra steps were necessary (in order to make the CI work again), here's the link to our merge request for future reference: https://gitlab.com/collectivecloud/collectives/-/merge_requests/301

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews dependencies Pull requests that update a dependency file enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants