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

devops/1410 #1411

Closed
wants to merge 2 commits into from
Closed

devops/1410 #1411

wants to merge 2 commits into from

Conversation

n8rzz
Copy link
Member

@n8rzz n8rzz commented Jul 13, 2019

Resolves #1412.

  • removes server/index.js and express dependencies
  • moves npm start to use npx http-server instead
  • removes references to server in gulp scripts
  • removes unused dependencies

* moves npm start to use npx http-server instead
* removes references to server in gulp scripts
* removes unused dependencies
@n8rzz n8rzz added the devops About developer tools label Jul 13, 2019
@n8rzz n8rzz added this to the v6.15.0 milestone Jul 13, 2019
@n8rzz n8rzz requested a review from a team July 13, 2019 21:00
@erikquinn erikquinn temporarily deployed to openscope-dev-pr-1411 July 13, 2019 21:01 Inactive
Copy link
Member

@erikquinn erikquinn left a comment

Choose a reason for hiding this comment

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

This here nerd stuff is beyond the capabilities of my pea brain... You say it's done and it fixes the issue?

@n8rzz
Copy link
Member Author

n8rzz commented Jul 14, 2019

It should fix the issue, yes. But I’m on a Mac, so I have no way to test this on windows. It always worked for me 😉

Anyone willing and able to just checkout the branch, delete public run the build and then start the app?

@JPmAn24
Copy link
Contributor

JPmAn24 commented Jul 15, 2019

Just took a look at this, and things are looking good. I'm currently on vacation so can't check if it actually fixes the deletion of index.js until I get back late on Thursday.

Thanks for doing this @n8rzz!

@erikquinn erikquinn self-requested a review July 15, 2019 20:16
@erikquinn
Copy link
Member

erikquinn commented Jul 19, 2019

Still trying to look for a solution here. It initially seems like this doesn't solve it, but that doesn't mean it might not still be the better approach-- it's just that the same issue of files not being copied correctly still persists. Here's a step-by-step process below which I followed to reliably see some behaviors.

When you have further input/advise/changes, it'll be easy for me to go back through them and see if we are then getting the correct result here. For now, I can't find a way to get this PR to load on localhost, while with some manual copying, I can get develop to do so. Anyway, the processes are below:


Process on develop:

  1. Manually delete node_modules, and run npm cache verify, then npm install.
  2. Delete ./public folder.
  3. Do npm run build and npm run start.
  4. Observe the failure, stating "Error: Cannot find module './public/assets/scripts/server/index.js'"`.
  5. Manually copy ./src/assets/scripts/server/index.js to ./public/assets/scripts/server/.
  6. Do npm run start.
  7. Observe the server running and the page being served.
  8. Stop the server (Ctrl+C), re-run npm run build and npm run start.
  9. Observe the failure message again.

Process on devops/1410:

  1. Manually delete node_modules, and run npm cache verify, then npm install.
  2. Delete ./public folder.
  3. Do npm run build and npm run start.
  4. Observe the server start, but no page be served at localhost:3003. The public folder contains no file named index.js, including in any subfolders.
  5. Observe that navigating to ./public/index.html and opening the file shows the loading screen but never finishes load.
  6. Copying ./src/assets/scripts/client/index.js to ./public/assets/scripts/client/ or ./public/assets/scripts/server/ do not yield a page which loads any more fully.
  7. Unsure how to proceed from here.

@n8rzz
Copy link
Member Author

n8rzz commented Jul 19, 2019

Hmm 🤔

Previous state

  • A small Express server was used to serve all assets during development
  • a gulp task existed to simply copy that simple server from src to public

New state

  • we use an npm package called http-server which does, essentially, the same thing: acts a simple server to server up assets during development
  • we never actually install this dep, instead we use npx. this means we can run a thing without having to actually install it
    * http-server defaults to serving a public dir, so you should be able to go to localhost:3003 and be done
  • on a mac, this fires up right away just like it did before.

This whole issue stemmed from a windows issue in the first place. I wonder if this introduces a new, different windows issue? What sort of output do you see after npm run start?

Screen Shot 2019-07-18 at 9 45 27 PM

@wrksx
Copy link
Contributor

wrksx commented Aug 6, 2019

I just tried it here on windows 8.1 + cygwin and it didn't work as expected.
It starts the server, and I can see HTTP requests logged on the server output.

Error using Chrome browser:

ERR_INVALID_REDIRECT

Error using Firefox browser:

Corrupted Content Error

The site at http://127.0.0.1:3003/ has experienced a network protocol violation that cannot be repaired.

The page you are trying to view cannot be shown because an error in the data transmission was detected.

Error using wget command:

$ wget http://127.0.0.1:3003/
--2019-08-06 15:02:02--  http://127.0.0.1:3003/
Connecting to 127.0.0.1:3003... connected.
HTTP request sent, awaiting response... 302 Found
Location: // [following]
http://: Invalid host name.

@wrksx
Copy link
Contributor

wrksx commented Aug 6, 2019

So this issue is known and referenced here: http-party/http-server#525
If i understand correctly, the temporarily recommended fix is to revert back to http-server@0.9.0.

I tested manually by running npx http-server@0.9.0 -p 3003 -c-1 and it worked.

I believe it's not a good idea to use npx http-server without explicitly depending on it. I think it's a
better practice to use the devDependencies to specify we depend on http-server@0.9.0 that way the version is fixed. The npm start script would still work fine and use the locally installed version.

I tested with "http-server": "0.9.0" in the devDependencies by running npm run start and it worked.

@n8rzz
Copy link
Member Author

n8rzz commented Aug 6, 2019

In most cases I would agree with you, this should live in devDependencies. However, in this case, I think it makes sense to use npx for these reasons:

  • this server is only needed to run the app locally
  • it's a convenience, a developer could use any local server to run the app
  • it reduces additional dependencies (because npx) and speeds up installs
  • one of my goals here is to remove the local server (and it's dependencies) entirely

I like calling out a specific version, that's generally a good practice anyway. I'd lean towards updating the command in this PR like you posted above referencing the version.

@erikquinn
Copy link
Member

The initial suggestion from @wrksx seems to resolve the issue. Recommend we go with that (posted PR #1415), and close this or otherwise update it once that fix goes in.

@erikquinn erikquinn removed their request for review August 7, 2019 20:50
@erikquinn erikquinn modified the milestones: v6.15.0, v6.16.0 Dec 1, 2019
@erikquinn
Copy link
Member

Seems to no longer be needed. Closing.

@erikquinn erikquinn closed this Jan 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devops About developer tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

upgrade deps and npm audit, v2
4 participants