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

publish to npm #27

Merged
merged 3 commits into from
Nov 24, 2017
Merged

publish to npm #27

merged 3 commits into from
Nov 24, 2017

Conversation

alanmshelly
Copy link
Contributor

@alanmshelly alanmshelly commented Nov 21, 2017

Warning: package name is not yet decided.

I tested to make sure the npm deploy command works properly on circleci

@alanmshelly alanmshelly mentioned this pull request Nov 21, 2017
@alanmshelly alanmshelly force-pushed the topic/npm branch 3 times, most recently from 55b862b to 4d3ddbd Compare November 21, 2017 06:33
@alanmshelly alanmshelly changed the title [WIP] enable npm publish to npm Nov 21, 2017
package.json Outdated
"main": "dist/skyway.js",
"module": "src/peer.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

Did this line cause any problems?

See more about what this line does -> https://stackoverflow.com/questions/42708484/what-is-the-module-package-json-field-for
But I don't think it reachs de facto standard yet. So it is no problem to remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no I don't think it caused any problems. it just seemed unnecessary so I removed it. I'll add it back in (I'll rebase it to clean up the commit log before it get's merged)

@KensakuKOMATSU
Copy link
Contributor

Shouldn't it be needed to delete /dist/ line from .gitignore?

I tried npm install by specifying this branch and repository.

$ npm install --save https://github.com/skyway/skyway-js-sdk.git#topic/npm

But it does not work.

$ cat main.js
const Skyway = require('skyway-js')
$ node main.js
module.js:491
    throw err;
    ^

Error: Cannot find module 'skyway-js'
    at Function.Module._resolveFilename (module.js:489:15)
    at Function.Module._load (module.js:439:25)
    at Module.require (module.js:517:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/Users/komasshu/development/filestest/main.js:1:78)
    at Module._compile (module.js:573:30)
    at Object.Module._extensions..js (module.js:584:10)
    at Module.load (module.js:507:32)
    at tryModuleLoad (module.js:470:12)
    at Function.Module._load (module.js:462:3)

I think this is due to no /dist/skyway.js in the node_modules trees.

$ tree node_modules/skyway-js/
node_modules/skyway-js/
├── CHANGELOG.md
├── LICENSE
├── README.md
└── package.json

@alanmshelly
Copy link
Contributor Author

Shouldn't it be needed to delete /dist/ line from .gitignore?

No, we will use npm publish on CircleCI to publish after we build so it isn't necessary.

I guess it is a disadvantage that we can't install from the github repo though.
I will try playing around with scripts in package.json to see if we can't build on install.

@KensakuKOMATSU
Copy link
Contributor

KensakuKOMATSU commented Nov 22, 2017

I have misunderstood the behavior of npm publish. My current understanding is /dist/skyway.js will be published when it is specified in files event though it is in .gitignore.

This behavior is make sense. I believe current PR should not be tweaked ;)

I guess responding to the topic that installing via npm from specified repository is not needed. This method should be used when repository is not npm published or easy-test.

@alanmshelly alanmshelly requested review from riosu and removed request for y-i November 24, 2017 00:45
Copy link
Contributor

@riosu riosu left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@iwashi
Copy link
Contributor

iwashi commented Nov 24, 2017

LGTM 😄

@iwashi iwashi merged commit caee980 into v1.1.0 Nov 24, 2017
@iwashi iwashi deleted the topic/npm branch November 24, 2017 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants