#2407 Add yarn serve #2618

larsroettig · 2020-08-10T18:12:39Z

Description

Start a test server yarn venia start
Start a production server NODE_ENV=production yarn venia start

Preconditions

pwa-studio/packages/venia-concept/.env need to exits
yarn build need to be executed

Related Issue

Closes #2407

Acceptance

Verification Stakeholders

Specification

Verification Steps

Start a test server should work
1.1. Forgetting run build should throw an error
yarn run watch:venia should work

Screenshots / Screen Captures (if appropriate)

Checklist

I have added tests to cover my changes, if necessary.

I have updated the documentation accordingly, if necessary.

PWAStudioBot · 2020-08-10T18:25:59Z

	Fails
🚫	A version label is required. A maintainer must add one.

	Messages
📖	Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.
📖	DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).

Generated by 🚫 dangerJS against 8e7b539

devops-pwa-codebuild · 2020-08-10T20:42:11Z

Performance Test Results

The following fails have been reported by WebpageTest. These numbers indicates a possible performance issue with the PR which requires further manual testing to validate.

https://pr-2618.pwa-venia.com : LH Performance Expected 0.85 Actual 0.33, LH Accessibility Expected 1 Actual 0.97, LH Best Practices Expected 1 Actual 0.93, WPT GZip Expected 100 Actual 26, WPT Cache Expected 90 Actual 39.333333333333
https://pr-2618.pwa-venia.com/venia-tops.html : LH Performance Expected 0.75 Actual 0.37, LH Best Practices Expected 1 Actual 0.93, WPT GZip Expected 100 Actual 26
https://pr-2618.pwa-venia.com/valeria-two-layer-tank.html : LH Performance Expected 0.8 Actual 0.36, LH Accessibility Expected 0.9 Actual 0.89, LH Best Practices Expected 1 Actual 0.93, WPT GZip Expected 100 Actual 26, WPT Cache Expected 65 Actual 50

lbajsarowicz · 2020-08-11T10:58:47Z

@larsroettig Adding export makes the variable visible in the console till you close it.

If you want it to be available just for that single command, there's a shorter way:

NODE_ENV=production yarn stage:venia

zetlen

First of all, thank you 🏆 for taking the initiative and making a new Buildpack CLI command. This looks great, and having a default staging script for all PWAs is very important for doing zero-configuration deploys as a Node server.

I have a couple of requested changes, and one general change which is large enough that I'm going to talk about it offline with you, and it can wait until later.

Also: Did you delete packages/pwa-buildpack/lib/WebpackTools/plugins/__tests__/UpwardDevServerPlugin.spec.js on purpose? We should update the test, not remove it.

package.json

packages/pwa-buildpack/lib/Utilities/serve.js

packages/pwa-buildpack/lib/WebpackTools/plugins/UpwardDevServerPlugin.js

packages/pwa-buildpack/lib/Utilities/serve.js

packages/venia-concept/package.json

zetlen

Everything looks great! I have just one request for a wording change and then i will fully approve.

packages/pwa-buildpack/lib/cli/serve.js

Update package.json Co-authored-by: James Zetlen <[email protected]> Update packages/pwa-buildpack/lib/Utilities/serve.js Co-authored-by: James Zetlen <[email protected]> Update packages/pwa-buildpack/lib/WebpackTools/plugins/UpwardDevServerPlugin.js Co-authored-by: James Zetlen <[email protected]> Update packages/venia-concept/package.json Co-authored-by: James Zetlen <[email protected]> Update packages/pwa-buildpack/lib/Utilities/serve.js Co-authored-by: James Zetlen <[email protected]>

tjwiebell

Code change looks good, but it appears it may have uncovered an existing bug that has broken scaffolding. @zetlen - is this error message accurate, that these upward definition names are illegal? I was trying to avoid doing explicit injection and just loop over the entire veniaSecurityHeaders collection, but didn't consider - was an illegal name. I'm relatively certain we merged some PR that started allowing these names for this specific purpose 🤔

edit: This PR definitely has the fix I'm expecting.

tjwiebell · 2020-08-19T16:13:53Z

@larsroettig @zetlen I've tracked down the problem. Because upward-js is no longer a dependency of venia-concept, it's no longer packing it for use by DEBUG_PROJECT_CREATION, so pwa-buildpack ends up using the latest published version, which doesn't have this fix. I'm not super comfortable having local scaffolding being broken until the next release, but I'm not sure there's an alternative. Any ideas for accepting this change while keeping local scaffolding in working shape?

larsroettig · 2020-08-19T20:22:01Z

@larsroettig @zetlen I've tracked down the problem. Because upward-js is no longer a dependency of venia-concept, it's no longer packing it for use by DEBUG_PROJECT_CREATION, so pwa-buildpack ends up using the latest published version, which doesn't have this fix. I'm not super comfortable having local scaffolding being broken until the next release, but I'm not sure there's an alternative. Any ideas for accepting this change while keeping local scaffolding in working shape?

@zetlen any idea how I can fix this bug that scaffolding still works after the merge?

The Buildpack `createProject` utility was only linking locally to PWA Studio packages that it specifically listed in its package.json. Other packages that were transitive dependencies were not detected and linked. That caused a [regression](#2618 (comment)) in #2618. This PR alters the scaffolding code so it adds explicit dependencies for every local dev-branch package. I also simplified and commented the code as I was auditing it. I also added some unit test coverage so that I could do more robust testing on `createProject`.

tjwiebell

Solution for transient deps is working perfectly 👌 thank you @zetlen and @larsroettig for resolving that. Sending to QA.

Merge pull request #1 from magento/develop

b2265cb

magento-engcom-team added the Partner: TechDivision label Aug 10, 2020

magento-engcom-team added the partners-contribution label Aug 10, 2020

PWAStudioBot added pkg:pwa-buildpack pkg:venia-concept labels Aug 10, 2020

PWAStudioBot added the pkg:upward-js Pertains to upward-js reference implementation of UPWARD. label Aug 10, 2020

larsroettig marked this pull request as ready for review August 11, 2020 08:16

zetlen suggested changes Aug 11, 2020

View reviewed changes

m2-community-project bot assigned zetlen Aug 11, 2020

m2-community-project bot added the Progress: changes requested label Aug 11, 2020

zetlen suggested changes Aug 12, 2020

View reviewed changes

packages/pwa-buildpack/lib/cli/serve.js Outdated Show resolved Hide resolved

zetlen self-requested a review August 13, 2020 16:04

zetlen previously approved these changes Aug 13, 2020

View reviewed changes

Merge branch 'develop' into 2407-yarn-serve

788d779

tjwiebell suggested changes Aug 19, 2020

View reviewed changes

m2-community-project bot assigned tjwiebell Aug 19, 2020

m2-community-project bot added Priority: P3 Severity: S2 labels Aug 19, 2020

James Zetlen and others added 2 commits August 21, 2020 12:02

Merge pull request #9 from magento/zetlen/yarn-serve-scaffolding-fix

6cadc09

larsroettig dismissed zetlen’s stale review via 6cadc09 August 23, 2020 15:24

tjwiebell approved these changes Aug 24, 2020

View reviewed changes

Lars Roettig and others added 2 commits August 24, 2020 23:40

Merge branch 'develop' into 2407-yarn-serve

e929d5b

Merge branch 'develop' into 2407-yarn-serve

8e7b539

dpatil-magento merged commit 60b3a34 into magento:develop Aug 26, 2020

m2-community-project bot removed the Progress: changes requested label Aug 26, 2020

larsroettig added Award: advanced Award: test coverage labels Oct 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#2407 Add yarn serve #2618

#2407 Add yarn serve #2618

larsroettig commented Aug 10, 2020 •

edited

Loading

PWAStudioBot commented Aug 10, 2020 •

edited

Loading

devops-pwa-codebuild commented Aug 10, 2020 •

edited

Loading

lbajsarowicz commented Aug 11, 2020

zetlen left a comment

zetlen left a comment

tjwiebell left a comment •

edited

Loading

tjwiebell commented Aug 19, 2020

larsroettig commented Aug 19, 2020 •

edited

Loading

tjwiebell left a comment

#2407 Add yarn serve #2618

#2407 Add yarn serve #2618

Conversation

larsroettig commented Aug 10, 2020 • edited Loading

Description

Preconditions

Related Issue

Acceptance

Verification Stakeholders

Specification

Verification Steps

Screenshots / Screen Captures (if appropriate)

Checklist

PWAStudioBot commented Aug 10, 2020 • edited Loading

devops-pwa-codebuild commented Aug 10, 2020 • edited Loading

lbajsarowicz commented Aug 11, 2020

zetlen left a comment

Choose a reason for hiding this comment

zetlen left a comment

Choose a reason for hiding this comment

tjwiebell left a comment • edited Loading

Choose a reason for hiding this comment

tjwiebell commented Aug 19, 2020

larsroettig commented Aug 19, 2020 • edited Loading

tjwiebell left a comment

Choose a reason for hiding this comment

larsroettig commented Aug 10, 2020 •

edited

Loading

PWAStudioBot commented Aug 10, 2020 •

edited

Loading

devops-pwa-codebuild commented Aug 10, 2020 •

edited

Loading

tjwiebell left a comment •

edited

Loading

larsroettig commented Aug 19, 2020 •

edited

Loading