feat(dev): Make PWADevServer host/SSL optional #175
Conversation
Pull Request Test Coverage Report for Build 1016
💛 - Coveralls |
|
Looks good. Will want to add our previous "happy path" to docs for those whose conditions these do meet in the future under a separate issue. Thanks! |
| * Emulate the public path settings of the Magento store | ||
| * Automatically switch domain names in HTML attributes | ||
| * Debug or disable ServiceWorkers | ||
| ## Basic Features |
| * Automatically switch domain names in HTML attributes | ||
| * Debug or disable ServiceWorkers | ||
| ## Basic Features | ||
| PWADevServer creates a `devServer` which is optimized for a Magento API-backed |
There was a problem hiding this comment.
PWADevServer creates an optimized
devServerfor Magento API-backed PWA development.The
devServerprovides the following useful features:
| PWADevServer creates a `devServer` which is optimized for a Magento API-backed | ||
| PWA development workflow, including: | ||
|
|
||
| * **Hot reload in the browser whenever you save a change** |
There was a problem hiding this comment.
Suggested rewrite:
Hot reload
The hot reload feature refreshes the whole page whenever you save a change that affects it. It uses Webpack's Hot Module Replacement feature to replace components and stylesheets inline.
There was a problem hiding this comment.
Slightly changed this because HMR does both the refresh and the inline replacement.
| * Using the [Hot Module Replacement](https://webpack.js.org/concepts/hot-module-replacement/) | ||
| feature of Webpack, replace components and stylesheets inline | ||
| * Refresh the whole page when a change requires it | ||
| * **Proxy API and media requests to Magento** |
There was a problem hiding this comment.
Suggested rewrite:
Proxy server
The
devServeracts as a proxy server for API and media requests to Magento. It is configured using environment variables.The
MAGENTO_BACKEND_DOMAINenvironment variable configures the proxy server to accept GraphQL and media requests and passes them to Magento.The
MAGENTO_BACKEND_PUBLIC_PATHenvironment variable allows the proxy server to serve static resources and JavaScript files at the same URL where Magento would serve them.The proxy server also transforms host and referral headers to make them compatible with Magento settings.
| * Use the `MAGENTO_BACKEND_PUBLIC_PATH` environment variable to serve | ||
| static resources and JS at the conventional URL where Magento would serve them | ||
| * Transform host and referral headers to be compatible with Magento settings | ||
| * **Serve Root level ServiceWorker** |
There was a problem hiding this comment.
Suggested rewrite:
Root level ServiceWorker
The
devServerserves a JavaScript file at the root path that registers a ServiceWorker scoped to the whole website. It can also disable that ServiceWorker when caching would interfere with realtime changes.
| * Update operating system security settings to trust the self-signed | ||
| certificate *(requires elevated permissions, so you may be asked for a | ||
| password)* | ||
| * **Content transformation** |
There was a problem hiding this comment.
Suggested rewrite:
Content transformation
The content transformation feature masks the Magento 2 domain name in all HTML attributes.
| | `publicPath: string` | **Required.** The public path to the theme assets in the backend server. | | ||
| | `backendDomain: string` | **Required.** The URL of the backend store. | | ||
| | `paths:`[`LocalProjectLocation`] | **Required.** Describes the location of the public static assets directory and where to deploy JavaScript files. | | ||
| | `serviceWorkerFileName: string` | **Required.** The name of the ServiceWorker file this theme creates, such as `sw.js`. | | ||
| | `changeOrigin: boolean` | **Experimental.** Toggles the [change origin feature]. Defaults to `false`. | | ||
| | `provideSSLCert: boolean` | *Optional*. Toggles the [create SSL certificate] feature. Set `true` to create an SSL certificate for the dev server, *and* to configure the OS and browser to trust the certificate it possible. **Requires temporary administrative access** to trust the certificate. |
There was a problem hiding this comment.
- Should be
**Optional.** - Remove comma before
*and* - " certificate it possible" => "certificate if possible"
- Can probably remove the part about requiring admin access to trust the certificate since it's already mentioned earlier.
| | `publicPath: string` | **Required.** The public path to the theme assets in the backend server. | | ||
| | `backendDomain: string` | **Required.** The URL of the backend store. | | ||
| | `paths:`[`LocalProjectLocation`] | **Required.** Describes the location of the public static assets directory and where to deploy JavaScript files. | | ||
| | `serviceWorkerFileName: string` | **Required.** The name of the ServiceWorker file this theme creates, such as `sw.js`. | | ||
| | `changeOrigin: boolean` | **Experimental.** Toggles the [change origin feature]. Defaults to `false`. | | ||
| | `provideSSLCert: boolean` | *Optional*. Toggles the [create SSL certificate] feature. Set `true` to create an SSL certificate for the dev server, *and* to configure the OS and browser to trust the certificate it possible. **Requires temporary administrative access** to trust the certificate. | ||
| | `provideUniqueHost: string|boolean` | *Optional*. Toggles the [create custom hostname] feature. Set `true` to create a unique hostname (made from the theme name and a hash of the project working directory). Or, set as a custom string, e.g. `"my-special-pwa"`, to override the default behavior of detecting the theme name from `package.json`. **Requires temporary administrative access** to add custom domain to the OS hostfile. |
There was a problem hiding this comment.
- Should be
**Optional.** - Description should be kept short. Details for the feature should be moved to the linked section.
| | `changeOrigin: boolean` | **Experimental.** Toggles the [change origin feature]. Defaults to `false`. | | ||
| | `provideSSLCert: boolean` | *Optional*. Toggles the [create SSL certificate] feature. Set `true` to create an SSL certificate for the dev server, *and* to configure the OS and browser to trust the certificate it possible. **Requires temporary administrative access** to trust the certificate. | ||
| | `provideUniqueHost: string|boolean` | *Optional*. Toggles the [create custom hostname] feature. Set `true` to create a unique hostname (made from the theme name and a hash of the project working directory). Or, set as a custom string, e.g. `"my-special-pwa"`, to override the default behavior of detecting the theme name from `package.json`. **Requires temporary administrative access** to add custom domain to the OS hostfile. | ||
| | `id: string` | *Optional*. Toggles and customizes the [create custom hostname] feature. Create a custom hostname exactly from the ID string, without attempting to hash the directory to ensure uniqueness. This option implies `provideUniqueHost`, but overrides the autogenerated subdomain. |
| | `provideSSLCert: boolean` | *Optional*. Toggles the [create SSL certificate] feature. Set `true` to create an SSL certificate for the dev server, *and* to configure the OS and browser to trust the certificate it possible. **Requires temporary administrative access** to trust the certificate. | ||
| | `provideUniqueHost: string|boolean` | *Optional*. Toggles the [create custom hostname] feature. Set `true` to create a unique hostname (made from the theme name and a hash of the project working directory). Or, set as a custom string, e.g. `"my-special-pwa"`, to override the default behavior of detecting the theme name from `package.json`. **Requires temporary administrative access** to add custom domain to the OS hostfile. | ||
| | `id: string` | *Optional*. Toggles and customizes the [create custom hostname] feature. Create a custom hostname exactly from the ID string, without attempting to hash the directory to ensure uniqueness. This option implies `provideUniqueHost`, but overrides the autogenerated subdomain. | ||
| | `changeOrigin: boolean` | *Experimental.* Toggles the [change origins in HTML] feature. Defaults to `false`. | |
There was a problem hiding this comment.
- Shoulde be
**Experimental.**
|
|
zetlen
force-pushed
the
zetlen/pwadevserver-optional-features
branch
2 times, most recently
from
854f94c to
391009f
Compare
|
@jcalcaben Revised my docs per your comments. Thank you! I like them much better now. |
| debug.errorMsg( | ||
| `findFreeHostname: Unable to find a free hostname after 9 tries. You may want to delete your database file at ${GlobalConfig.getDbFilePath()} to clear out old developer hostname entries. (Soon we will make this easier and more automatic.)` | ||
| ) | ||
| `createUniqueSubdomain(): Using default "${name}" prefix. Could not autodetect theme name from package.json` |
There was a problem hiding this comment.
The name of this method appears to be getUniqueSubdomain, not createUniqueSubdomain.
There was a problem hiding this comment.
Perils of using static refactoring tools. Thanks!
| const pkg = require(pkgLoc); | ||
| if (!pkg.name) { | ||
| throw new Error( | ||
| `package.json has ${pkg.name} "name" field!` |
There was a problem hiding this comment.
Could we rewrite this error to be a little more clear?
There was a problem hiding this comment.
The way I've structured this, that error gets immediately caught, and then handled along with any other errors that might occur trying to require() the package.json file. So the errors that throw will look like:
Error loading package.json:
getUniqueSubdomain: Using default "my-pwa" prefix. Could not autodetect theme name from package.json Error: cannot find module './package.json'`
package.json exists but somehow has no "name" field:
`getUniqueSubdomain: Using default "my-pwa" prefix. Could not autodetect theme name from package.json Error: package.json has undefined "name" field!`
I can add a comment explaining this if you like, but I think the errors that emit are clear enough.
There was a problem hiding this comment.
No, my concern was that if pkg.name is falsy but not undefined, the sentence won't make much sense, since this assumes it's an adjective. 😅
If undefined is the only possible falsy value for pkg.name, then this doesn't need to be a template string. But let's just make it a bit more precise and flexible.
// pkg.name: ''
package.json has "name" field!
// alternative
package.json is missing a "name" value!
There was a problem hiding this comment.
I had not considered the null string. Changing!
| .toLowerCase() | ||
| .replace(/[^a-zA-Z0-9]/g, '-') | ||
| .replace(/^-+/, ''); | ||
| }, |
| devHost = await PWADevServer.provideCustomHost(config.id); | ||
| } else if (config.provideUniqueHost) { | ||
| devHost = await PWADevServer.provideUniqueHost( | ||
| config.id || config.provideUniqueHost |
There was a problem hiding this comment.
Wouldn't this statement always evaluate to true, given the conditions?
There was a problem hiding this comment.
It's actually possibly a string. But I changed the behavior so that config.id and config.provideUniqueHost are mutually exclusive (and config.id is just preserving legacy behavior), so I don't need the first part of that conditional./
zetlen
force-pushed
the
zetlen/pwadevserver-optional-features
branch
3 times, most recently
from
cfd5330 to
c5cb391
Compare
|
@jimbo Fixed that error message for ya. |
zetlen
force-pushed
the
zetlen/pwadevserver-optional-features
branch
from
c5cb391 to
db16407
Compare
|
Closes #8. |
zetlen
force-pushed
the
zetlen/pwadevserver-optional-features
branch
from
a937c54 to
9918327
Compare
zetlen
force-pushed
the
zetlen/pwadevserver-optional-features
branch
from
8e05557 to
e74d19d
Compare
|
Closes #5. |
PWADevServer attempts to create a unique domain name, create local SSL certificates, and tell the OS to trust those certificates, for every new project. It also tries to confirm they exist on every run! This causes many problems for users under some common conditions: - No administrative access to local machine - No OpenSSL installed, or wrong OpenSSL installed - OS cannot be scripted to trust certificates - Developer uses Firefox, which uses its own cert store Additionally, some bugs in the implementation have caused some developers' projects to enter an unusable state. - Adds `provideUniqueHost` flag to PWADevServer configuration. PWADevServer will no longer try to create or retrieve a custom domain name unless `provideUniqueHost` is in its configuration in `webpack.config.js` as either a custom string or `true`. - Adds `provideSSLCert` flag to PWADevServer configuration. PWADevServer will no longer try to create or retrieve a trusted SSL certificate unless `provideSSLCert: true` is in its configuration in `webpack.config.js`. - Modifies custom domain name creation strategy to ensure uniqueness based on a hash of the full local path, rather than using the local flat file database. We created these features for the needs of the developer working on several PWAs at once on their local machine, so that they don't have to set up manual SSL every time, and they have no conflicts with Service Workers. This could be considered "bonus functionality", as it's not critical to the setup of a minimum viable PWA. It was meant to establish our focus on developer experience, and articulate the parts of developer setup that PWA Studio can "own". *However, we soon learned that we could not maintain all scenarios for automated setup and continue to make progress with shopper-facing features*. We still really want to support and automate all of these scenarios, but for now, our implementations are a hindrance and we are turning them off by default. fixup: Documentation edits from PR feedback
zetlen
force-pushed
the
zetlen/pwadevserver-optional-features
branch
from
4f13bba to
9bfe081
Compare
|
Went ahead and merged this to take some work of Jimmy's plate--it's already been extensively reviewed. |
* Check if captureStackTrace exists before calling (#288) * Check if captureStackTrace exists before calling * Add test to ensure captureStackTrace isn't called when it does not exist * Prettier ✨ * Fix failing test (#304) Unit tests should use MemoryRouter, not BrowserRouter, and do shallow rendering rather than full mounting. * feat(dev): Make PWADevServer host/SSL optional (#175) PWADevServer attempts to create a unique domain name, create local SSL certificates, and tell the OS to trust those certificates, for every new project. It also tries to confirm they exist on every run! This causes many problems for users under some common conditions: - No administrative access to local machine - No OpenSSL installed, or wrong OpenSSL installed - OS cannot be scripted to trust certificates - Developer uses Firefox, which uses its own cert store Additionally, some bugs in the implementation have caused some developers' projects to enter an unusable state. - Adds `provideUniqueHost` flag to PWADevServer configuration. PWADevServer will no longer try to create or retrieve a custom domain name unless `provideUniqueHost` is in its configuration in `webpack.config.js` as either a custom string or `true`. - Adds `provideSSLCert` flag to PWADevServer configuration. PWADevServer will no longer try to create or retrieve a trusted SSL certificate unless `provideSSLCert: true` is in its configuration in `webpack.config.js`. - Modifies custom domain name creation strategy to ensure uniqueness based on a hash of the full local path, rather than using the local flat file database. We created these features for the needs of the developer working on several PWAs at once on their local machine, so that they don't have to set up manual SSL every time, and they have no conflicts with Service Workers. This could be considered "bonus functionality", as it's not critical to the setup of a minimum viable PWA. It was meant to establish our focus on developer experience, and articulate the parts of developer setup that PWA Studio can "own". *However, we soon learned that we could not maintain all scenarios for automated setup and continue to make progress with shopper-facing features*. We still really want to support and automate all of these scenarios, but for now, our implementations are a hindrance and we are turning them off by default. fixup: Documentation edits from PR feedback * chore: manually set version 1.1.0 and lerna fix (#305)
PWADevServer attempts to create a unique domain name, create local
SSL certificates, and tell the OS to trust those certificates, for
every new project. It also tries to confirm they exist on every run!
This causes many problems for users under some common conditions:
Additionally, some bugs in the implementation have caused some
developers' projects to enter an unusable state.
This change makes those features optional and adds clearer documentation around them.
This PR is a:
[ ] New feature
[ ] Enhancement/Optimization
[x] Refactor
[ ] Bugfix
[ ] Test for existing code
[ ] Documentation
Summary
provideUniqueHostflag to PWADevServer configuration.PWADevServer will no longer try to create or retrieve a custom domain
name unless
provideUniqueHostis in its configuration inwebpack.config.jsas either a custom string ortrue.provideSSLCertflag to PWADevServer configuration. PWADevServerwill no longer try to create or retrieve a trusted SSL certificate
unless
provideSSLCert: trueis in its configuration inwebpack.config.js.based on a hash of the full local path, rather than using the local
flat file database.
Additional information
We created these features for the needs of the developer working on
several PWAs at once on their local machine, so that they don't have to
set up manual SSL every time, and they have no conflicts with Service
Workers. This could be considered "bonus functionality", as it's not
critical to the setup of a minimum viable PWA. It was meant to establish
our focus on developer experience, and articulate the parts of developer
setup that PWA Studio can "own".
However, we soon learned that we could not maintain all scenarios for
automated setup and continue to make progress with shopper-facing
features. We still really want to support and automate all of these
scenarios, but for now, our implementations are a hindrance and we are
turning them off by default.