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

Migrate to node 16, npm 7. #3773

Merged
merged 25 commits into from
Oct 14, 2021
Merged

Conversation

dvkruchinin
Copy link
Contributor

@dvkruchinin dvkruchinin commented Oct 6, 2021

Resolve #2742

Motivation and context

Migration to node version 16 and npm version 7.
Transferring commonly used packages to the root package.json (if possible).
Updating Dockerfile to use the root package.json. Updating Dockerfile.ci. Updating Dockerfile.ui to use stretch image which uses node 16.
Updating pipeline files. Updating CONTRIBUTING guide.

How has this been tested?

Checklist

License

  • I submit my code changes under the same MIT License that covers the project.
    Feel free to contact the maintainers if that's a concern.
  • I have updated the license header for each file (see an example below)
# Copyright (C) 2021 Intel Corporation
#
# SPDX-License-Identifier: MIT

@dvkruchinin
Copy link
Contributor Author

Hi @bsekachev,
please take a look to the patch.
Unfortunately, I do not understand the reason for the snyk tests error. node-sass had to be updated because to work with node 16, the required version of ``node-sass``` 6+.

@nmanovic
Copy link
Contributor

nmanovic commented Oct 7, 2021

Hi @bsekachev, please take a look to the patch. Unfortunately, I do not understand the reason for the snyk tests error. node-sass had to be updated because to work with node 16, the required version of ``node-sass``` 6+.

@dvkruchinin , @bsekachev , I will suggest to replace node-sass. What do you think?

Warning: LibSass and Node Sass are deprecated. While they will continue to receive maintenance releases indefinitely, there are no plans to add additional features or compatibility with any new CSS or Sass features. Projects that still use it should move onto Dart Sass.

@bsekachev
Copy link
Member

bsekachev commented Oct 7, 2021

@nmanovic @dvkruchinin

node-sass is a dependency of sass-loader. Sass loader documentation says we can use dart-sass or node-sass, so, let's replace node-sass, I do not see problems here.

@dvkruchinin Can you do it?

https://github.com/sass/dart-sass#from-npm

@dvkruchinin
Copy link
Contributor Author

@bsekachev

Can you do it?

https://github.com/sass/dart-sass#from-npm

I`ll take a look. In case of problems, I will contact you.

@dvkruchinin
Copy link
Contributor Author

dvkruchinin commented Oct 7, 2021

scss files has been affected due to the occurrence of warning (as an example of one of)

  ╷
9 │ $layout-sm-grid-size: $grid-unit-size / 2;
  │                       ^^^^^^^^^^^^^^^^^^^
  ╵
    src/base.scss 9:23                                        @import
    src/components/update-cloud-storage-page/styles.scss 5:9  root stylesheet

DEPRECATION WARNING: Using / for division is deprecated and will be removed in Dart Sass 2.0.0.

Recommendation: math.div($grid-unit-size, 2)

More info and automated migrator: https://sass-lang.com/d/slash-div

To fix it, I used https://sass-lang.com/documentation/breaking-changes/slash-div#automatic-migration

@bsekachev
Copy link
Member

Hi,

We need to define workspaces (actually it was the key idea of migration).
In root package.json add:

  "workspaces": ["cvat-data", "cvat-core", "cvat-canvas", "cvat-canvas3d", "cvat-ui", "tests"],

After that we need only one npm ci in root directory which installs all the dependencies.
Also need to update this file with relevant installation steps: cvat/site/content/en/docs/contributing/development-environment.md

@bsekachev
Copy link
Member

Also with new npm we can probably move all the package.json commands (like npm start, npm build, etc) (from /cvat-ui, /cvat-core, /cvat-canvas, etc) to the root. To run them need specify workspace. Please refer to documentation: https://docs.npmjs.com/cli/v7/using-npm/workspaces#running-commands-in-the-context-of-workspaces

And describe all of them in cvat/README.md with a short description

@dvkruchinin
Copy link
Contributor Author

Added workspaces to root package.json. Added the appropriate commands to run build, server.

@bsekachev
Copy link
Member

Hi @dvkruchinin

The patch looks fine, but I come across one issue. When I open typescript files from cvat-canvas, cvat-canvas3d,
Eslint says:

[Info  - 1:24:47 PM] Error while loading rule '@typescript-eslint/dot-notation': You have used a rule which requires parserServices to be generated. You must therefore provide a value for the "parserOptions.project" property for @typescript-eslint/parser. Occurred while linting /home/user/cvat/cvat-canvas3d/src/typescript/canvas3d.ts

If I do some changes in these files, they are automatically linted on precommit hook and linter fails with the same error, so, I can't commit if these files are changed. Could you please look into this?

RUN npm ci
# Install common dependencies
WORKDIR /tmp/
RUN npm ci --ignore-scripts
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need --ignore-scripts flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this flag I got an error rm: cannot remove '.husky/pre-commit': No such file or directory

@dvkruchinin
Copy link
Contributor Author

Hi @bsekachev,

Error Error while loading rule '@typescript-eslint/dot-notation' fixed.

@bsekachev bsekachev merged commit e484aa7 into cvat-ai:develop Oct 14, 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.

Migration from NPM 6 to NPM 7
3 participants