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

fix(module-map): missing baseUrl to module map on server #206

Merged
merged 7 commits into from
Jun 24, 2020

Conversation

infoxicator
Copy link
Contributor

Fixes an issue where the baseUrl key was present on the moduleMap cached sent to the client but not present on the server module map.

Description

The baseUrl key is used by one-app-ducks to determine the url where the language pack statics are stored. However, when doing Server Side Rendering, the baseUrl is not present and it was falling back to the client rendering to load the language pack (breaking server side rendering) when using either loadLanguagePack or queryLanguagePack

Motivation and Context

This change fixes this issue by taking the node bundle type for each module and creating the baseUrl from the url in the node bundle key.

How Has This Been Tested?

This is a non-breaking bugfix that solves an issue with loadLanguagePack

This solution uses the browser key in the module map which is available on both the clientModuleMapCache and the moduleMap already available on the server.

Tested this solution with queryLanguageData and a clean module generated with intl, the language pack now loads on the server on the first render

Component rerender: 1
LangpackTest.jsx:16 languageData => {locale: "en-US", greeting: "Welcome to langpack-test from the United States!"}
LangpackTest.jsx:17 isLoading => false

Tested with cultured-frankie and loadLanguageData however, even though the language pack loads on the server on the first render, the moduleStatus property only changes from loading to loaded on the second render, this has been identified as a separate issue and will be addressed on a different PR.

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation (adding or updating documentation)
  • Dependency update

Checklist:

  • My change requires a change to the documentation and I have updated the documentation accordingly.
  • These changes should be applied to a maintenance branch.
  • This change requires cross browser checks.
  • Performance tests should be ran against the server prior to merging.
  • This change impacts caching for client browsers.
  • This change impacts HTTP headers.
  • This change adds additional environment variable requirements for One App users.
  • I have added the Apache 2.0 license header to any new files created.

What is the Impact to Developers Using One App?

No expected impact

@infoxicator infoxicator requested review from a team as code owners June 19, 2020 16:20
@oneamexbot
Copy link
Contributor

oneamexbot commented Jun 19, 2020

📊 Bundle Size Report

file name size on disk gzip
app.js 275.3KB 82.2KB
runtime.js 15KB 5.4KB
vendors.js 117.2KB 36.4KB
app~vendors.js 432.4KB 112.2KB
service-worker-client.js 17KB 5.2KB
legacy/app.js 290.6KB 85.3KB
legacy/runtime.js 15KB 5.4KB
legacy/vendors.js 163.4KB 44.7KB
legacy/app~vendors.js 441.3KB 114.5KB
legacy/service-worker-client.js 17.5KB 5.4KB

Generated by 🚫 dangerJS against 0e758c6

JAdshead
JAdshead previously approved these changes Jun 23, 2020
mtomcal
mtomcal previously approved these changes Jun 23, 2020
...acc,
[moduleName]: {
...moduleBundles,
baseUrl: moduleBundles.baseUrl ? moduleBundles.baseUrl : moduleBundles.node.url.replace(/[^/]+\.js$/i, ''),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this option should be documented

nellyk
nellyk previously approved these changes Jun 24, 2020
@JAdshead JAdshead merged commit c6a251e into master Jun 24, 2020
@JAdshead JAdshead deleted the fix/module-map-base-url branch June 24, 2020 17:53
@JAdshead JAdshead added the fix label Jun 24, 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.

7 participants