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

Add ws and socket.io-client packages to the Node.js runtime #523

Closed
wants to merge 1 commit into from

Conversation

jberstler
Copy link

@jberstler jberstler commented May 26, 2016

Resolves #268

@jberstler
Copy link
Author

@markusthoemmes based on the history of the Dockerfile, you seem like the right person to assign this to. Please re-assign if needed.

@rabbah
Copy link
Member

rabbah commented May 26, 2016

@bjustin-ibm need to update refrences.md
FYI @csantanapr

@jberstler jberstler force-pushed the wspackages branch 2 times, most recently from 20b44d3 to 40acc35 Compare May 26, 2016 18:28
val (out, err) = withNodeJsContainer { c =>
val code = """
| function main(args) {
| require('wildlyuniquenameofanonexistentpackage');
Copy link
Author

Choose a reason for hiding this comment

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

Good point.

Copy link

@dfederschmidt dfederschmidt May 26, 2016

Choose a reason for hiding this comment

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

But for now, thats not an issue as we are controlling which packages are available - so it was a mistake on my side - looks good to me otherwise :)
EDIT: somehow my first comment got deleted, I am on mobile.. I was just pointing out that this test might fail if someone registers "wildlyuniquename.." as a module

val (out, err) = withNodeJsContainer { c =>
val code = """
| function main(args) {
| require('.mildlyinvalidnameofanonexistentpackage');
Copy link
Author

Choose a reason for hiding this comment

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

@danielfederschmidt I think your point is still technically valid, and the change was easy enough to make, so I went ahead and did it 😉

@csantanapr
Copy link
Member

csantanapr commented May 26, 2016

@rabbah @bjustin-ibm created an issue to track change that need to sync with bluemix docs.
IBM-Cloud/docs#273

@jberstler
Copy link
Author

PG 324

@markusthoemmes
Copy link
Contributor

LGTM, is this good to merge documentation wise?

@csantanapr
Copy link
Member

csantanapr commented May 28, 2016

@markusthoemmes yes, this is ready for merge.
Tracking sync to bluemix docs is being tracked like I mentioned in #523 (comment)

@markusthoemmes
Copy link
Contributor

rebased and merged 364e61a

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants