Skip to content
This repository has been archived by the owner on Aug 3, 2023. It is now read-only.

Suggest re-building node_modules on failed builds where appropriate #428

Closed
xortive opened this issue Aug 12, 2019 · 3 comments · Fixed by #837
Closed

Suggest re-building node_modules on failed builds where appropriate #428

xortive opened this issue Aug 12, 2019 · 3 comments · Fixed by #837
Assignees
Labels
design feature Feature requests and suggestions
Milestone

Comments

@xortive
Copy link
Contributor

xortive commented Aug 12, 2019

We assume that the existence of node_modules in a webpack project means the dependencies are installed and we don't need to run npm install again. However, if the user does ctrl-c during wrangler build, wrangler publish or wrangler preview while the npm install stage is running, it is possible to have a node_modules folder with invalid contents.

@xortive xortive added bug Something isn't working status - PR welcome labels Aug 12, 2019
@EverlastingBugstopper
Copy link
Contributor

ooh good catch. do you think it's worth running npm install on every build or is there a better way to check for an unfinished install?

@gusvargas
Copy link
Contributor

gusvargas commented Oct 7, 2019

I've never developed/published a worker before so sorry if I'm misunderstanding something about the workflow... Wouldn't eagerly npm install-ing make it possible for your dependencies to shift between build/publishes/previews (assuming a lock file isn't used or isn't capturing all transitive dependencies)? If so, I think it that would be a bit surprising – I wouldn't expect build/publish/preview to have side-effects outside of what the command describes.

Seems like the question that needs to be answered is "is something in node_modules wrong?" – yarn check --verify-tree can help with that even if yarn wasn't used to install the dependencies originally. It should be easy enough to shell out to it and observe the exit code or process its stdout. If an issue was detected I'd probably want to be prompted for permission before wrangler ran the install process itself.

If I'm missing some context then I apologize for the distraction.

/cc @ashleymichal

Related to #765

@ashleymichal
Copy link
Contributor

for the moment, we should try outputting a helpful message when these errors occur, such as "encountered error running npm install; try removing DIR/node_modules and running again" or "try running yarn check --verify-tree", etc.

@ashleymichal ashleymichal changed the title Ctrl-C during wranglerjs initiated NPM install breaks future builds Suggest re-building node_modules on failed builds where appropriate Oct 29, 2019
@EverlastingBugstopper EverlastingBugstopper added feature Feature requests and suggestions and removed bug Something isn't working labels Nov 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
design feature Feature requests and suggestions
Projects
None yet
7 participants
@xortive @gusvargas @ashleygwilliams @ashleymichal @gabbifish @EverlastingBugstopper and others