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

Feature/meteor 1.3 update #944

Merged
merged 26 commits into from
May 26, 2016
Merged

Feature/meteor 1.3 update #944

merged 26 commits into from
May 26, 2016

Conversation

brylie
Copy link
Contributor

@brylie brylie commented May 20, 2016

Closes #927
Closes #935
Closes #939
Closes #940
Closes #941
Closes #942
Closes #949

Suggested changes

  • Update all possible project dependencies, including Meteor
  • add package.json for NPM dependency management
  • remove unneeded packages
    • switch NPM package handling

@brylie
Copy link
Contributor Author

brylie commented May 20, 2016

@frenchbread, @elnzv, and @jykae will you all please test this PR?

@frenchbread pay particular attention to the dashboard, such as the getChartData method. I have moved the elasticsearch dependency to package.json, and removed the meteorhacks:npm wrapper package.

@brylie brylie added the WIP label May 20, 2016
@55
Copy link
Contributor

55 commented May 20, 2016

Getting this in server console on /dashboard

 Exception while invoking method 'getChartData' TypeError: Object [object Object] has no method 'npmRequire'
     at new ElasticRest (packages/frenchbread_elastic-rest/server/elastic-rest.js:33:1)
     at [object Object].getChartData (server/methods/elasticSearch.js:48:16)
     at maybeAuditArgumentChecks (packages/ddp-server/livedata_server.js:1704:12)
     at packages/ddp-server/livedata_server.js:711:19
     at [object Object]._.extend.withValue (packages/meteor/dynamics_nodejs.js:56:1)
     at packages/ddp-server/livedata_server.js:709:40
     at [object Object]._.extend.withValue (packages/meteor/dynamics_nodejs.js:56:1)
     at packages/ddp-server/livedata_server.js:707:46

@brylie
Copy link
Contributor Author

brylie commented May 20, 2016

TypeError: Object [object Object] has no method 'npmRequire'
at new ElasticRest (packages/frenchbread_elastic-rest/server/elastic-rest.js

OK, our NPM dependency handling has changed, so we probably need to use the ES2015 import syntax in that file.

@frenchbread would you please try to resolve this issue? You can commit directly to this branch.

@jykae
Copy link
Contributor

jykae commented May 20, 2016

Could we bring elastic-rest under Apinf organization if not there already? just a side note..

@jykae
Copy link
Contributor

jykae commented May 24, 2016

@brylie Got yesterday's Github exception removed by commenting out https://github.com/apinf/api-umbrella-dashboard/blob/develop/server/accounts.js#L60 onLogin handler. From history it looks like this might be some legacy code, was still able to login with Github. Consider cleaning up.

@brylie
Copy link
Contributor Author

brylie commented May 24, 2016

Good catch @jykae

@brylie
Copy link
Contributor Author

brylie commented May 24, 2016

We need to deprecate meteorhacks:npm from any project dependencies. @frenchbread will you see if you can update the elastic-rest module, so that it uses NPM directly?

@frenchbread
Copy link
Contributor

@brylie Yep. Had that idea in mind. Will do.

@brylie
Copy link
Contributor Author

brylie commented May 24, 2016

@frenchbread we are currently refactoring the dashboard code. I will push it to this branch right now.

@brylie
Copy link
Contributor Author

brylie commented May 24, 2016

@frenchbread we have refactored the getChartData server method, which is working as of now. The last part that is giving us trouble, is getting the data into the charts, from within the method callback. Please review, and fix if possible, the client/views/dashboard/charts/charts.js

@jykae
Copy link
Contributor

jykae commented May 24, 2016

@brylie @frenchbread
Notes from the evening coder: I think it works now, we just forgot to return the chained result. Added dc from npm

@55
Copy link
Contributor

55 commented May 25, 2016

Getting this error on startup:

=> Errors prevented startup:

   While selecting package versions:
   error: unknown package in top-level dependencies: npm-container

=> Your application has errors. Waiting for file change.

If I remove this package manually it will work correctly.
Should I commit those changes and push it in this branch?

@frenchbread
Copy link
Contributor

Working on #972 #949 since they are blocking this one

@brylie brylie removed the WIP label May 25, 2016
@brylie brylie added this to the Sprint 23 milestone May 25, 2016
@brylie
Copy link
Contributor Author

brylie commented May 25, 2016

@frenchbread please see my comment in #949

@55 55 merged commit a444f83 into develop May 26, 2016
@55 55 deleted the feature/meteor-1.3-update branch May 26, 2016 08:45
@55 55 removed the in progress label May 26, 2016
@as33ms as33ms mentioned this pull request May 26, 2016
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