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

Update: npm packages to latest versions #472

Merged
merged 13 commits into from
Nov 9, 2017
Merged

Conversation

pramodsum
Copy link
Contributor

@pramodsum pramodsum commented Nov 7, 2017

Packages that still need to be updated

  • sinon

Copy link
Contributor

@JustinHoldstock JustinHoldstock left a comment

Choose a reason for hiding this comment

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

Great stuff!

@@ -365,7 +365,7 @@ class DocBaseViewer extends BaseViewer {
* @return {void}
*/
setPage(pageNumber) {
if (pageNumber < 1 || pageNumber > this.pdfViewer.pagesCount) {
if (!pageNumber || pageNumber < 1 || pageNumber > this.pdfViewer.pagesCount) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch!

@@ -116,7 +111,7 @@
"prettier": "prettier-eslint \"src/lib/**/*.js\" --print-width 120 --single-quote --tab-width 4 --write",
"prod": "BABEL_ENV=production NODE_ENV=production node --max_old_space_size=4096 ./node_modules/webpack/bin/webpack.js --progress --colors --config build/webpack.config.js",
"release": "yarn run clean && yarn run build-rb && yarn run lint && yarn run test && yarn run prod",
"test": "NODE_ENV=test ./node_modules/.bin/karma start build/karma.conf.js",
"test": "NODE_ENV=test node --max_old_space_size=4096 ./node_modules/.bin/karma start build/karma.conf.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way we can tell if we have a memory leak? Or is this just a reasonable amount of memory to run tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a temporary solution while we look into ways to optimize the unit tests. We shouldn't be hitting this limit (especially after annotations has been removed). I'm going to go through the tests and verify that they're all being cleaned up properly.

@pramodsum pramodsum merged commit 26e8d7b into box:master Nov 9, 2017
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.

3 participants