Update appengine_tasks to include playground dependencies #4591
Conversation
| } | ||
|
|
||
| /** | ||
| * Copies playground deps into deploy directory. |
There was a problem hiding this comment.
This copy differs from the other copyStaticSrc and copyAppengineSrc since it copies uncommited local files. I think it's worth mentioning in the JsDoc about that (and perhaps mention that npm install needs to have been run).
If possible, I'd like for us to check that there is node_modules directory somewhere in this step because otherwise it will just silently "fail" if someone was to have cleaned their node modules before running.
There was a problem hiding this comment.
Hmm, all good points. If the goal is to not have it depend on the current state of the repo, perhaps we can directly acquire the files we're interested in from unpkg.
There was a problem hiding this comment.
Also I noticed that the script reads from the local package.json to determine the version, rather than the one in HEAD. Should that also be switched over?
There was a problem hiding this comment.
The current deploy demo script uses files in HEAD, rather than local files because it was a direct translation of the steps we have done in the past. I want to note that we could consider changing it.
Assuming we're not:
- unpkg: +1 to using unpkg, assuming it's not too complicated (not necessary for this PR, but we could file an issue to add later)
package.json: good catch. I think it probably would be better to use the version in HEAD
There was a problem hiding this comment.
In the interest of not letting this sit, I'm going to throw this in, and update the associated but with the remaining tasks.
The basics
The details
Resolves
Fixes #4567
Proposed Changes
Include playground deps from node_modules in the set of files deployed to appengine.
Reason for Changes
Fix deployed playgrounds.
Test Coverage
Tested on:
Documentation
Additional Information