Skip to content
This repository has been archived by the owner on May 3, 2024. It is now read-only.

fix(utils): replace watchLocalModules cdn-url string to be ip/port #17

Merged
merged 2 commits into from
Jan 23, 2020

Conversation

stephaniecoates
Copy link
Contributor

after generating a module, running npm run watch:build and serving it locally in One App, the follow error would appear:

log: Failed to load Holocron module at [one-app-dev-cdn-url]/static/modules/my-module/1.0.0/yeehaw.node.js
log: Error: only absolute urls are supported

this fix replaces the static [one-app-dev-cdn-url] string and replaces it with the actual ip and port when pulling from the module map.

Note: The module still loaded successfully before this fix - this is specifically to mitigate the error in the console.

@stephaniecoates stephaniecoates force-pushed the fix/pass-accurate-one-app-dev-cdn-url branch from 9398309 to 0ccdae0 Compare January 22, 2020 22:19
@oneamexbot
Copy link
Contributor

oneamexbot commented Jan 22, 2020

📊 Bundle Size Report

file name size on disk gzip
app.js 112.4KB 31.3KB
runtime.js 15KB 5.3KB
vendors.js 128KB 37.9KB
app~vendors.js 403.8KB 105.7KB
legacy/app.js 119KB 32.9KB
legacy/runtime.js 15KB 5.3KB
legacy/vendors.js 163.1KB 44.9KB
legacy/app~vendors.js 410.2KB 107.4KB

☑️ Preflight Checklist:

All questions must be addressed before this PR can be merged.

  • What is the communication plan for this change?
  • Does any documentation need to be updated or added to account for this change? If so was it done already?
  • What is the motivation for this change?
  • Should these changes also be applied to a maintenance branch or any other long lived branch?
  • How was this change tested?
  • Does this change require cross browser checks? Why or why not?
  • Does this change require a performance test prior to merging? Why or why not?
  • Could this be considered a breaking change? Why or why not?
  • Does the change impact caching?
  • Does the change impact HTTP headers?
  • Does the change have any new infrastructure requirements?
  • Does the change affect other versions of the app?
  • Does the change require additional environment variables?
  • What is the impact to tenants?
  • What is the impact to individual users?
  • What is the change to the size of assets?
  • Should integration tests be added to protect against future regressions on this change?

Generated by 🚫 dangerJS against 69c86b0

@stephaniecoates
Copy link
Contributor Author

What is the communication plan for this change?
None, just fixes a console error when watching a reloading module changes
Does any documentation need to be updated or added to account for this change? If so was it done already?
No, change is in line with current documentation
What is the motivation for this change?
To mitigate console errors
Should these changes also be applied to a maintenance branch or any other long lived branch?
No
How was this change tested?
Wrote unit tests to validate module replacing dynamic url from module map
Does this change require cross browser checks? Why or why not?
No, local development fix only
Does this change require a performance test prior to merging? Why or why not?
No, local development fix only
Could this be considered a breaking change? Why or why not?
No, local development fix only
Does the change impact caching?
No, local development fix only
Does the change impact HTTP headers?
No, local development fix only
Does the change have any new infrastructure requirements?
No, local development fix only
Does the change affect other versions of the app?
No, local development fix only
Does the change require additional environment variables?
No, local development fix only
What is the impact to tenants?
Less unnecessary errors in console
What is the impact to individual users?
No, local development fix only
What is the change to the size of assets?
None, local development fix only
Should integration tests be added to protect against future regressions on this change?
No, local development fix only

@anescobar1991 anescobar1991 merged commit 0650b16 into master Jan 23, 2020
@anescobar1991 anescobar1991 deleted the fix/pass-accurate-one-app-dev-cdn-url branch January 23, 2020 22:14
@nellyk nellyk added the fix label Apr 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants