This repository has been archived by the owner on May 15, 2024. It is now read-only.
feat(host): flag to use req.host instead of localhost #14
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mtomcal
previously approved these changes
May 7, 2020
|
Do we need to mention something in the Readme? |
JAdshead
reviewed
May 7, 2020
| @@ -57,6 +57,7 @@ export default ({ | |||
| remoteModuleMapUrl, | |||
| useLocalModules, | |||
| appPort, | |||
| useHost, | |||
| }) => { | |||
There was a problem hiding this comment.
what about adding something like below here. you can probably come up with a better name however
Suggested change
| }) => { | |
| }) => { | |
| const hostAddress = useHost ? `http://${req.headers.host}` : `http://localhost:${process.env.HTTP_ONE_APP_DEV_CDN_PORT}`; |
There was a problem hiding this comment.
had to put it inside the get for the req to be accessible but Implemented your changes here: 1c5a17c
src/index.js
Outdated
| @@ -106,7 +107,7 @@ export default ({ | |||
| const remoteModuleMap = JSON.parse(r.body); | |||
|
|
|||
| const { modules } = remoteModuleMap; | |||
| const oneAppDevStaticsAddress = `http://localhost:${process.env.HTTP_ONE_APP_DEV_CDN_PORT}/static`; | |||
| const oneAppDevStaticsAddress = useHost ? `http://${req.headers.host}/static` : `http://localhost:${process.env.HTTP_ONE_APP_DEV_CDN_PORT}/static`; | |||
There was a problem hiding this comment.
continued from comment above -
Suggested change
| const oneAppDevStaticsAddress = useHost ? `http://${req.headers.host}/static` : `http://localhost:${process.env.HTTP_ONE_APP_DEV_CDN_PORT}/static`; | |
| const oneAppDevStaticsAddress = `${hostAddress}/static`; |
this change can also be applied below
src/index.js
Outdated
| @@ -141,7 +142,7 @@ export default ({ | |||
| : {}, | |||
| (useLocalModules ? getLocalModuleMap({ | |||
| pathToModulemap: path.join(localDevPublicPath, 'module-map.json'), | |||
| oneAppDevCdnAddress: `http://localhost:${process.env.HTTP_ONE_APP_DEV_CDN_PORT}`, | |||
| oneAppDevCdnAddress: useHost ? `http://${req.headers.host}}` : `http://localhost:${process.env.HTTP_ONE_APP_DEV_CDN_PORT}`, | |||
There was a problem hiding this comment.
Suggested change
| oneAppDevCdnAddress: useHost ? `http://${req.headers.host}}` : `http://localhost:${process.env.HTTP_ONE_APP_DEV_CDN_PORT}`, | |
| oneAppDevCdnAddress: hostAddress, |
Added documentation to README 1c5a17c |
infoxicator
commented
May 11, 2020
| @@ -84,13 +84,23 @@ location where the remote module map is located (i.e. `https://my-domain.com/map | |||
|
|
|||
| Type: `boolean` | |||
|
|
|||
| Default: `true`<br> | |||
| Whether to use modules from `localDevPublicPath`. Passed as `true` or `false`, defaults to `true`. | |||
| Default: `false`<br> | |||
There was a problem hiding this comment.
In the code this never defaulted to true so I updated the README to reflect
JAdshead
approved these changes
May 12, 2020
mtomcal
approved these changes
May 12, 2020
oneamexbot
added a commit
that referenced
this pull request
May 13, 2020
# [3.3.0](v3.2.2...v3.3.0) (2020-05-13) ### Features * **host:** flag to use req.host instead of localhost ([#14](#14)) ([5e761a7](5e761a7))
|
🎉 This PR is included in version 3.3.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In order for the browser tests to run the dev-cdn the module map has to point to the host instead of localhost. I have added the flag
useHostto be set to true when running browser tests and false as default when using one-app-runner so it points to localhost during local development.Tested by passing the flag from one-app, building a dev docker image, passing the image local to one-app-runner and running the browser tests. both scenarios worked as expected