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

Added http-server script #971

Merged
merged 3 commits into from
Mar 8, 2023
Merged

Added http-server script #971

merged 3 commits into from
Mar 8, 2023

Conversation

ankur12-1610
Copy link
Contributor

Fixes: #969

@ankur12-1610
Copy link
Contributor Author

@Jaifroid kindly review :)

Also, we might need to update the documentation as well: this is the link of the same

@Jaifroid
Copy link
Member

Jaifroid commented Mar 4, 2023

@ankur12-1610 Thank you for this PR. I tested by running npm install --save-dev and then npm start, and the server does indeed start. However, I already have http-server installed globally. Do you know if this would work when it is not installed? I would have thought we'd need to have it listed as a developer dependency in the json, but I haven't tested that scenario. It could be tested in Windows Sandbox or another clean testbed.

Regarding your suggestion to update the documentation, I think that would be part of this PR, but should be done once the above question is tested.

Finally, I'm not sure npm start is the best choice because it is the standard script for launching an Electron app, and Electron is enabled downstream in kiwix-js-windows repository (which depends on this repository).

Perhaps serve would be better, and then the start command would be npm run serve. What do you think?

@Jaifroid Jaifroid added dependencies Pull requests that update a dependency file build Code relating to building, publishing, or maintaining the app tests labels Mar 4, 2023
@ankur12-1610
Copy link
Contributor Author

@ankur12-1610 Thank you for this PR. I tested by running npm install --save-dev and then npm start, and the server does indeed start. However, I already have http-server installed globally. Do you know if this would work when it is not installed? I would have thought we'd need to have it listed as a developer dependency in the json, but I haven't tested that scenario. It could be tested in Windows Sandbox or another clean testbed.

Regarding your suggestion to update the documentation, I think that would be part of this PR, but should be done once the above question is tested.

Finally, I'm not sure npm start is the best choice because it is the standard script for launching an Electron app, and Electron is enabled downstream in kiwix-js-windows repository (which depends on this repository).

Perhaps serve would be better, and then the start command would be npm run serve. What do you think?

Hey @Jaifroid, I've tested it in MacOS and it works perfectly fine in it. I'll it in try other operating systems as well.

Regarding the dependency, we already have included http-server in package.json.

Screenshot from 2023-03-06 02-26-24

Also, updated the PR now it is npm run serve and I've updated the docs as well.

The only final step left is verification :)

Copy link
Member

@Jaifroid Jaifroid left a comment

Choose a reason for hiding this comment

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

@ankur12-1610 Thank you, this looks good. I tested launching the server also, and it seems to work fine.

Do you know whether http-server gets installed if a user hasn't installed it globally? You had originally mentioned perhaps needing to run it with npx. I wasn't able to test this because I have it installed globally.

I have one suggestions below. You can use the GitHub buttons to commit the suggestion if you think it's OK.

CONTRIBUTING.md Outdated Show resolved Hide resolved
@ankur12-1610
Copy link
Contributor Author

@ankur12-1610 Thank you, this looks good. I tested launching the server also, and it seems to work fine.

Do you know whether http-server gets installed if a user hasn't installed it globally? You had originally mentioned perhaps needing to run it with npx. I wasn't able to test this because I have it installed globally.

I have one suggestions below. You can use the GitHub buttons to commit the suggestion if you think it's OK.

Yeah I tested it without installing http-server globally in a new environment, npm install does the work for the user so using npx isn't necessary.

Sure I'll do it👍

Co-authored-by: Jaifroid <egk10@cam.ac.uk>
@Jaifroid
Copy link
Member

Jaifroid commented Mar 7, 2023

Great! So I think this can be squashed and merged. Just confirm that it's ready and I'll press the button (I assume you don't have access to the Squash and merge button on this page).

@ankur12-1610
Copy link
Contributor Author

Great! So I think this can be squashed and merged. Just confirm that it's ready and I'll press the button (I assume you don't have access to the Squash and merge button on this page).

Yeah LGTM! as well. Yeah I don't have access to squash and merge, although I do have an access to close PR xD

@Jaifroid Jaifroid merged commit 74690b8 into kiwix:main Mar 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Code relating to building, publishing, or maintaining the app dependencies Pull requests that update a dependency file tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding http-server run script
2 participants