Dependencies: Fix broken prod dependencies

[sirreal]

Changes proposed in this Pull Request:

• Drop production-only dependency installation. There's very limited benefit to skipping devDependency installation (save some time in an infrequent process) and it leads to issues that only occur when making the production bundle.
• Add all upgrade-nudge dependencies to dependencies. With upgrade-nudge SSR (Build: Server-side Render UpgradeNudge for use in PHP #13070), these dependencies will be required.

This was working because @wordpress/url is installed as a transitive dependency, @wordpress/e2e-test-utils (a devDependency) causes it to be installed. If @wordpress/e2e-test-utils (like with yarn install --production) - @wordpress/url is not installed.

I believe, this is only a problem with extensions that have "SSR" from the implementation in #13070, at the moment only Simple Payments. At runtime in the browser, @wordpress/* dependencies are externals, meaning they are pulled from the runtime environment. That makes the presence or absence of a @wordpress/* package irrelevant. However, with #13070 SSR, some of these dependencies will be required at build time.

See p1566905517230900-slack-luna / d871cf9

Testing instructions:

• yarn build-production builds without errors.
• Add a Simple Payments block to a site on a free plan. The upgrade nudge should be visible in the editor and on the frontend for admin users (see Build: Server-side Render UpgradeNudge for use in PHP #13070)

[matticbot]

Caution: This PR has changes that must be merged to WordPress.com
Hello sirreal! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D32074-code before merging this PR. Thank you!

[jetpackbot]

	Warnings
⚠️	pre-commit hook was skipped for one or more commits
⚠️	"Proposed changelog entry" is missing for this PR. Please include any meaningful changes

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 55702a8

[ockham]

I believe, this is only a problem with extensions that have "SSR" from the implementation in #13070, at the moment only Simple Payments. At runtime in the browser, @wordpress/* dependencies are externals, meaning they are pulled from the runtime environment. That makes the presence or absence of a @wordpress/* package irrelevant. However, with #13070 SSR, some of these dependencies will be required at build time.

Yes, that's the exact reason why I was adding those as devDeps.

[ockham]

• Add all upgrade-nudge dependencies to dependencies. With upgrade-nudge SSR (Build: Server-side Render UpgradeNudge for use in PHP #13070), these dependencies will be required.

Not entirely sure I'm following the rationale here. Should it not be enough to keep them as devDeps (and proably add @wordpress/url there) if we're dropping production-only dependency installation anyway? (My main point would be to use deps vs devDeps to visibly distinguish between deps that actually need to be installed, vs external ones.)

[sirreal]

• Add all upgrade-nudge dependencies to dependencies. With upgrade-nudge SSR (Build: Server-side Render UpgradeNudge for use in PHP #13070), these dependencies will be required.

Not entirely sure I'm following the rationale here. Should it not be enough to keep them as devDeps (and proably add @wordpress/url there) if we're dropping production-only dependency installation anyway? (My main point would be to use deps vs devDeps to visibly distinguish between deps that actually need to be installed, vs external ones.)

Yes, if we drop the production-only installation it doesn't matter where dependencies are declared. I'd remove devDependencies entirely and consider a lint rule to prohibit its use. That's my preference for simplicity and would obviate the rest of this response.

For an application like Jetpack which isn't a published package, there's no clear distinction about what dependencies and devDependencies are. It's one set of dependencies and a superset of those, we can make an arbitrary decision between dependencies and devDependencies.

At runtime, we don't need any dependencies. @zinigor mentioned that some devDependencies are large, take a lot of time and space to install, and cause problems with the beta builder. That would mean that one place we can make the dep/devDep separation is "necessary for building the application" or not. That aligns with Jetpack, the (unpublished) Jetpack npm app is really only serves the purpose of building assets for the Jetpack plugin. calypso-build is a dependency, it's required for running the core functionality of the app - building assets. devDependencies can become the extras that aren't essential for building the assets - things like e2e testing packages.

[ockham]

Okay, sounds like a good approach! 👍

[jeherve]

It works well as is. Merging.