Skip to content

Conversation

AzGoalie
Copy link
Contributor

What does this PR do?

Switches out NPM in favor of Yarn.

Who is reviewing it (please choose AT LEAST two reviewers that need to approve the PR before it can get merged)?

@brendan-hofmann @djblue @ahoffer

Select at least one member from relevant component team(s) from below (at least one component team member needs to approve the PR).

Build

Choose 2 committers to review/merge the PR (please choose ONLY two committers from below, delete the rest).

@pklinef
@stustison

How should this be tested? (List steps with links to updated documentation)

All OS: Complete build works
Windows: Quickbuild with no npm cache or yarn cache and all node_modules folders deleted is successful

Any background context you want to provide?

What are the relevant tickets?

Screenshots (if appropriate)

Checklist:

  • Documentation Updated
  • Change Log Updated
  • Update / Add Unit Tests
  • Update / Add Integration Tests

Copy link

@ahoffer ahoffer left a comment

Choose a reason for hiding this comment

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

That was a lot less change than I expected. Great.

@brendan-hofmann
Copy link
Contributor

Have you looked into the build failures at all?

@AzGoalie AzGoalie force-pushed the yarn branch 2 times, most recently from 8f155e1 to edfd42a Compare January 18, 2017 19:44
@@ -70,8 +70,8 @@
"webpack-merge": "0.14.0"
},
"dependencies": {
"@turf/circle": "3.3.3",
Copy link
Member

Choose a reason for hiding this comment

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

I am concerned about potential regressions from lowering the turf version. How was this regression tested?

It looks like public scoped packages should work with Yarn.
yarnpkg/yarn#825

Maybe we can try adding the namespace to the npmrc file for this project.
yarnpkg/yarn#839
@turf:registry=http://registry.npmjs.org/

@@ -7,7 +7,7 @@
"main": "index.js",
"scripts": {
"start": "hotreloadify src/main/webapp/js/index.js --proxy=https://localhost:8993 --open",
"pretest": "standard ./src/**/*.js",
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove this line?

@@ -226,18 +226,18 @@
<artifactId>frontend-maven-plugin</artifactId>
<executions>
<execution>
Copy link
Member

Choose a reason for hiding this comment

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

I think you can remove the entire execution.

@@ -21,7 +21,7 @@ define([
'terraformer',
'terraformer-wkt-parser',
'js/CQLUtils',
'@turf/turf',
Copy link
Member

Choose a reason for hiding this comment

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

Please revert these back.

Copy link
Member

@pklinef pklinef left a comment

Choose a reason for hiding this comment

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

We have some READMEs that talking about doing npm install. These should be updated.
https://github.com/codice/ddf/search?utf8=%E2%9C%93&q=%22npm+install%22&type=Code

@shaundmorris shaundmorris added this to the 2.10 milestone Jan 20, 2017
@@ -40,7 +40,7 @@ Automated tests can execute against a local Selenium server and target locally i
* Install and start a local Selenium server. [Webdrvr](https://www.npmjs.org/package/webdrvr) can automate this process.

```
npm install -g webdrvr
yarn install -g webdrvr
Copy link
Member

Choose a reason for hiding this comment

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

I think you need to do yarn global add webdrvr

https://yarnpkg.com/en/docs/cli/global

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was recently deprecated, will update.

@brendan-hofmann
Copy link
Contributor

Passed a full build on windows. Building on linux and then will do manual verification.

Copy link
Contributor

@brendan-hofmann brendan-hofmann left a comment

Choose a reason for hiding this comment

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

Built on linux as well. Just had to tell bower to allow root on my vm, since it runs as the root user.

DDF-2721: Removed npm-shrinkwrap

DDF-2721: Added mutex to install

DDF-2721: Updated readmes

DDF-2721: Updated yarn test order

DDF-2721: Rolled back standard version

DDF-2721: Removed unused dep and added node to catalog-ui
@pklinef pklinef merged commit c127223 into codice:master Jan 30, 2017
lavoywj pushed a commit to lavoywj/ddf that referenced this pull request Feb 13, 2020
DDF-2721: Removed npm-shrinkwrap

DDF-2721: Added mutex to install

DDF-2721: Updated readmes

DDF-2721: Updated yarn test order

DDF-2721: Rolled back standard version

DDF-2721: Removed unused dep and added node to catalog-ui
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.

7 participants