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

fix: add lock file and mention production usage #42

Merged
merged 14 commits into from
Jan 19, 2021

Conversation

Michsior14
Copy link
Contributor

No description provided.

README.md Outdated
@@ -4,6 +4,8 @@ Small server wrapper around Z-Wave JS to access it via a WebSocket.

## Trying it out

Keep in mind that on production you want to execute `npm run build` and use `node` instead of `ts-node` for all bellow commands.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Keep in mind that on production you want to execute `npm run build` and use `node` instead of `ts-node` for all bellow commands.
Keep in mind that on production you want to execute `npm run build` and use `node` instead of `ts-node` for all below commands.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't htink that this sentence makes sense.

In production, when installed from npm, it is already JS and they should use the CLIs that we install:

https://github.com/zwave-js/zwave-js-server/blob/master/package.json#L6-L9

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's true if installed via npm, but it might be worth to mention that ts-node is not preferred for running it in long term if someone decide to clone. Maybe the above sentence describes it wrongly, I would be happy to change it to something more meaningful ;)

@@ -1,5 +1,4 @@
node_modules
package-lock.json
Copy link
Collaborator

Choose a reason for hiding this comment

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

So this project can both be used as a lib and as a standalone server. When installed as a lib (ie as part of ZJS2MQTT), the package-lock file will be ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we ignore package-lock.json from git we basically allow to unintentionally bump the dependencies during npm i on github actions and contributors machines. This can lead to failed tests/builds.
In the end this file won't be published to npm by default, so it won't be a problem for the lib consumers.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should publish it to npm, for when it's installed as a CLI.

When an npm package is installed as a dependency, the package-lock file is already ignored anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can publish npm-shrinkwrap.json only. I will add the proper command to the github action.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry to jump in, but when I install this from npm (npm install @zwave-js/server), it's currently pulling in zwave-js 6.0.0-beta.3 as a dependency. Is that what this conversation is about? I don't think I should have that version as a dependency yet...

README.md Outdated
@@ -4,6 +4,8 @@ Small server wrapper around Z-Wave JS to access it via a WebSocket.

## Trying it out

Keep in mind that on production you want to execute `npm run build` and use `node` instead of `ts-node` for all below commands.
Copy link
Member

Choose a reason for hiding this comment

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

That's not correct. If you install this using the command npm install -g @zwave-js/server you will be able to run this by simply call zwave-server /dev/ttyACM0 and zwave-client taken from: https://github.com/zwave-js/zwave-js-server/blob/master/package.json#L7

README.md Outdated Show resolved Hide resolved
@balloob balloob merged commit 15cea50 into zwave-js:master Jan 19, 2021
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

5 participants