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

[replaced with #58] chore(package.json): Switch from yarn to npm and handle dependency upgrades #51

Closed
wants to merge 11 commits into from

Conversation

mturley
Copy link
Collaborator

@mturley mturley commented Jan 28, 2021

Switching us from yarn to npm to have consistent tooling with forklift-ui. WIP: I need to test npm link to see if these instructions are valid.

Edit: this PR previously handled dependency upgrades as well, we've addressed that in #57 so I'm closing this and opening a PR specific to the yarn-to-npm change.

@mturley mturley requested a review from a team January 28, 2021 18:53
@codecov
Copy link

codecov bot commented Jan 28, 2021

Codecov Report

Merging #51 (a8d5aca) into main (a3cae11) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##              main       #51   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines           19        19           
  Branches         5         5           
=========================================
  Hits            19        19           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a3cae11...a8d5aca. Read the comment docs.

@konveyor-preview-bot
Copy link

🚀 Deployed Storybook Preview: http://konveyor-lib-ui-pr-51-preview.surge.sh

@mturley
Copy link
Collaborator Author

mturley commented Jan 28, 2021

Converted to draft because I want to figure out the README bit about npm link before we merge.

@mturley
Copy link
Collaborator Author

mturley commented Jan 29, 2021

@gildub trying to test this via npm link in forklift-ui, webpack fails to start with this error (both in main and on your upgrade-webpack branch):

[WEBPACK] ERROR in ./node_modules/@konveyor/lib-ui/dist/index.es.js 4611:2
[WEBPACK] Module parse failed: Unexpected token (4611:2)
[WEBPACK] You may need an appropriate loader to handle this file type, currently no loaders are configured to process this file. See https://webpack.js.org/concepts#loaders
[WEBPACK] | BaseSchema.prototype.optional = BaseSchema.prototype.notRequired;
[WEBPACK] |
[WEBPACK] > {}.toString();
[WEBPACK] | // String Interfaces
[WEBPACK] | //
[WEBPACK]  @ ./src/app/Plans/components/VMMigrationDetails.tsx 7:0-53 117:13-30
[WEBPACK]  @ ./src/app/routes.tsx 16:0-74 80:19-37
[WEBPACK]  @ ./src/app/index.tsx 7:0-40 17:44-53
[WEBPACK]  @ ./src/index.tsx 3:0-33 17:36-39
[WEBPACK]
[WEBPACK] webpack 5.17.0 compiled with 1 error in 35809 ms
[WEBPACK] ℹ 「wdm」: Failed to compile.

I'm giving up for now, maybe we can troubleshoot together next week. I don't know if it is happening because of using npm link in general or because of the rollup upgrade I did in this PR.

@mturley
Copy link
Collaborator Author

mturley commented Apr 22, 2021

I'm now handling dependency upgrades in #57 before we replace yarn, so after that is merged we'll need to rebase or rewrite this PR to focus on the yarn-to-npm change.

mturley added a commit that referenced this pull request Apr 22, 2021
In order to support upgrading dependencies in forklift-ui and benefit from some bug fixes in dependencies, this PR upgrades all the things we should try to keep fresh: peer dependencies consumers also depend on and will keep up to date (patternfly, yup, react, react-dom), and core code quality and docs stuff (typescript, semantic-release, storybook, jest, testing-library, eslint, rollup, babel, commitizen, prettier)

This PR replaces/adopts a lot of the changes in #51, so that PR should be rebased or rewritten after this change so that it can focus on the yarn-to-npm change.

Notable upgrades include:
* TypeScript 3 to 4 (required the installation of `tslib` to handle some syntax)
* Storybook 6.0 to 6.2 (removes the requirement of a direct dependency on webpack, so this PR removes webpack),
* yup from 0.29.3 to 0.32.8 (propagates some breaking changes to consumers, see below)
* PatternFly from 2020.10 to 2021.04 (https://www.patternfly.org/v4/developer-resources/release-notes)
* React from 16.13 to 17.0 (we're not affected by any of their breaking changes)
* eslint from 7.6 to 7.24
* prettier 2.0 to 2.2
* rollup 2.23 to 2.45

BREAKING CHANGE: The yup upgrade bundles its own TS types, removing the need to depend on `@types/yup`, and some of the type changes are breaking. `yup.Schema` has been replaced by `yup.SchemaOf`, and it correctly infers array types now so we were able to remove the hacky `MaybeArraySchema` type from useFormState. There have also been some behavior changes in some of the schema methods, particularly the `.defined()` and `.required()` methods. See those changes here: https://github.com/jquense/yup/blob/master/CHANGELOG.md (the items between 0.29.3 and 0.32.8). We felt it was a good idea to upgrade even though this is a little disruptive, because the new built-in TS types are less finicky and more helpful than the community-provided ones were.
@mturley mturley changed the title chore(package.json): Switch from yarn to npm, upgrade most dependencies, adjust for breaking changes in yup types chore(package.json): Switch from yarn to npm Apr 22, 2021
@mturley mturley closed this Apr 22, 2021
@mturley mturley changed the title chore(package.json): Switch from yarn to npm chore(package.json): Switch from yarn to npm and handle dependency upgrades [replaced] Apr 22, 2021
@mturley mturley changed the title chore(package.json): Switch from yarn to npm and handle dependency upgrades [replaced] [replaced with #58] chore(package.json): Switch from yarn to npm and handle dependency upgrades Apr 22, 2021
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.

2 participants