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

Remove bower, upgrade lodash v3 to v4 #296

Merged
merged 31 commits into from
Jan 13, 2017
Merged

Conversation

cheapsteak
Copy link
Contributor

No description provided.

@dcc-jenkins
Copy link
Contributor

Deployed to https://dev.dcc.icgc.org/portals/299 for testing

@btiernay
Copy link
Contributor

btiernay commented Jan 5, 2017

Conflicts.

…/dccprtl-99-debowerize

# Conflicts:
#	dcc-portal-ui/README.md
#	dcc-portal-ui/yarn.lock
@cheapsteak
Copy link
Contributor Author

Merged with develop

@btiernay
Copy link
Contributor

btiernay commented Jan 5, 2017

@SidSyrus / @cheapsteak we need to coordinate this merge with any outstanding UI branches. Otherwise bad things will happen.

@SidSyrus
Copy link
Contributor

SidSyrus commented Jan 5, 2017

_.orderBy expects the sort order to be a string asc/desc while the old _.sortByOrder was expecting it to be a boolean true/false. You might have to change that as well.

@btiernay I think that was the reason why AS bar charts were not sorted.

image

@btiernay
Copy link
Contributor

btiernay commented Jan 5, 2017

Good catch @SidSyrus. I think we should review this more thoroughly before testing. Is there a migration guide?

@btiernay
Copy link
Contributor

btiernay commented Jan 5, 2017

Perhaps we should run this before and during this branch:

https://github.com/lodash/lodash-migrate

@cheapsteak
Copy link
Contributor Author

cheapsteak commented Jan 5, 2017

_.orderBy expects the sort order to be a string asc/desc while the old _.sortByOrder was expecting it to be a boolean true/false


http://codepen.io/anon/pen/pRvOwj

```js
// Using lodash 4.15.0:
const sample = _.range(10).map(x => ({y: _.random(0, 100)}));
// [{"y":88},{"y":7},{"y":60},{"y":68},{"y":26},{"y":55},{"y":23},{"y":19},{"y":21},{"y":57}]
const sortedSample = _.orderBy(sample, 'y', true);
// [{"y":7},{"y":19},{"y":21},{"y":23},{"y":26},{"y":55},{"y":57},{"y":60},{"y":68},{"y":88}]
```

~~~Although a through review wouldn't hurt~~~

edit:oh shit nvm, reverse order doesn't work

@cheapsteak
Copy link
Contributor Author

nvm realized reverse doesnt work

@cheapsteak
Copy link
Contributor Author

Added lodash-migrate, quite a few warnings, will attend

@btiernay
Copy link
Contributor

btiernay commented Jan 9, 2017

ok to test

@btiernay
Copy link
Contributor

btiernay commented Jan 9, 2017

Please remove bower from setup instructions. Thanks

@btiernay
Copy link
Contributor

btiernay commented Jan 9, 2017

@cheapsteak:

PhantomJS 2.1.1 (Linux 0.0.0) ERROR
 ReferenceError: Can't find variable: _
 at app/scripts/vendor.js:54

PhantomJS 2.1.1 (Linux 0.0.0): Executed 0 of 0 ERROR (0.58 secs / 0 secs)

PhantomJS 2.1.1 (Linux 0.0.0) ERROR
 ReferenceError: Can't find variable: angular
 at app/scripts/index.js:35214

PhantomJS 2.1.1 (Linux 0.0.0): Executed 0 of 0 ERROR (0.664 secs / 0 secs)

PhantomJS 2.1.1 (Linux 0.0.0) ERROR
 TypeError: Attempted to assign to readonly property.
 at node_modules/angular-mocks/angular-mocks.js:17

PhantomJS 2.1.1 (Linux 0.0.0): Executed 0 of 0 ERROR (0.667 secs / 0 secs)

npm ERR! Test failed.  See above for more details.

@cheapsteak
Copy link
Contributor Author

cheapsteak commented Jan 9, 2017

It's been removed image

Were there other spots?

@btiernay
Copy link
Contributor

btiernay commented Jan 9, 2017

Also, we have merged in develop so we should ensure that the merge didn't introduce old lodash usages:

525cab9

@btiernay
Copy link
Contributor

btiernay commented Jan 9, 2017

@cheapsteak Any idea why the build is failing after the merge?

@btiernay
Copy link
Contributor

btiernay commented Jan 9, 2017

Getting similar locally as well after cd dcc-portal-ui; rm -rf removing app/bower_components; mvn clean package:

  ReferenceError: Can't find variable: _
  at app/scripts/vendor.js:54

PhantomJS 2.1.1 (Mac OS X 0.0.0): Executed 0 of 0 ERROR (0.19 secs / 0 secs)

PhantomJS 2.1.1 (Mac OS X 0.0.0) ERROR
  ReferenceError: Can't find variable: angular
  at app/scripts/index.js:35214

PhantomJS 2.1.1 (Mac OS X 0.0.0): Executed 0 of 0 ERROR (0.204 secs / 0 secs)

PhantomJS 2.1.1 (Mac OS X 0.0.0) ERROR
  TypeError: Attempted to assign to readonly property.
  at node_modules/angular-mocks/angular-mocks.js:17

PhantomJS 2.1.1 (Mac OS X 0.0.0): Executed 0 of 0 ERROR (0.205 secs / 0 secs)```

@SidSyrus
Copy link
Contributor

SidSyrus commented Jan 9, 2017

https://github.com/icgc-dcc/dcc-portal/commit/525cab953fc2abda92fc088356ddf779abf27bcd#diff-8c320a23e8338d7fac7be5b254660618R63

~~~Lodash V4 doesn't have `_.contains` method. I have the fix for it in my [branch](https://github.com/icgc-dcc/dcc-portal/blob/feature/DCCPRTL-99-debowerize-sid-branch/dcc-portal-ui/app/scripts/stackedbarchart/js/directives.js#L63). Should I request a PR to merge into this branch?~~~

Edit: NVM. The build still fails in my branch as well even with the mentioned change.

@btiernay
Copy link
Contributor

btiernay commented Jan 9, 2017

@btiernay
Copy link
Contributor

btiernay commented Jan 9, 2017

Actually, the build started failing before the merge at f1c4fa0:

https://dcc-jenkins.oicr.on.ca/job/dcc-portal-pr/2809/org.icgc.dcc$dcc-portal-ui/console
image

https://dcc-jenkins.oicr.on.ca/job/dcc-portal-pr/2808/org.icgc.dcc$dcc-portal-ui/console

  ReferenceError: Can't find variable: _
  at app/scripts/vendor.js:54

PhantomJS 2.1.1 (Linux 0.0.0): Executed 0 of 0�[31m ERROR�[39m (0.813 secs / 0 secs)

�[31mPhantomJS 2.1.1 (Linux 0.0.0) ERROR�[39m
  ReferenceError: Can't find variable: angular
  at app/scripts/index.js:35214

PhantomJS 2.1.1 (Linux 0.0.0): Executed 0 of 0�[31m ERROR�[39m (0.892 secs / 0 secs)

�[31mPhantomJS 2.1.1 (Linux 0.0.0) ERROR�[39m
  TypeError: Attempted to assign to readonly property.
  at node_modules/angular-mocks/angular-mocks.js:17

PhantomJS 2.1.1 (Linux 0.0.0): Executed 0 of 0�[31m ERROR�[39m (0.895 secs / 0 secs)

npm ERR! Test failed.  See above for more details.```

@btiernay
Copy link
Contributor

Weird @codacy-bot, I wonder what makes you do the things you do when you do them.

@btiernay btiernay merged commit 910213b into develop Jan 13, 2017
@btiernay btiernay deleted the feature/dccprtl-99-debowerize branch January 13, 2017 20:26
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.

None yet

4 participants