Generate SSR source map with environment variable #1571

yunakim714 · 2023-11-28T18:39:26Z

Description

W-14148180
In order to improve the readability of server-side errors in MRT logs, we need to generate and output a source map as part of the bundle. This PR aims to write a new script so that customers can generate this file, ssr.js.map, only when they run the script: npm run push:ssr-source-map. This file will not be publicly exposed or available to download.

In order to then enable source maps in Cloud Watch logs, we need to set the environment variable, NODE_OPTIONS=--enable-source-maps in AWS Lambda.

Types of Changes

Bug fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Documentation update
Breaking change (could cause existing functionality to not work as expected)
Other changes (non-breaking changes that does not fit any of the above)

Breaking changes include:

Removing a public function or component or prop

Adding a required argument to a function

Changing the data type of a function parameter or return value

Adding a new peer dependency to package.json

Changes

Create a new script npm run push:ssr-source-map that runs build with the environment variable SSR_SOURCE_MAP
If SSR_SOURCE_MAP is set to true then build will generate and output a ssr.js.map file as part of the bundle

How to Test-Drive This PR

(step1)

Checklists

General

Changes are covered by test cases
CHANGELOG.md updated with a short description of changes (not required for documentation updates)

Accessibility Compliance

You must check off all items in one of the follow two lists:

There are no changes to UI

or...

Changes were tested with a Screen Reader (iOS VoiceOver or Android Talkback) and had no issues
Changes comply with WCAG 2.0 guidelines levels A and AA
Changes to common UI patterns and interactions comply with WAI-ARIA best practices

Localization

Changes include a UI text update in the Retail React App (which requires translation)

bredmond-sf · 2023-11-28T19:29:04Z

packages/template-retail-react-app/package.json

@@ -20,6 +20,7 @@
    "lint:fix": "npm run lint -- --fix",
    "postinstall": "npm run compile-translations && npm run compile-translations:pseudo",
    "push": "npm run build && pwa-kit-dev push",
+    "push:source-map": "cross-env SSR_SOURCE_MAP=true npm run build && pwa-kit-dev push",


I'd say push:ssr-source-map to be specific

Lol, said the opposite over here, but I'm unclear on the "why" behind this

#1571 (comment)

packages/template-retail-react-app/package.json

packages/pwa-kit-dev/src/configs/webpack/config.js

wjhsf · 2023-11-29T21:41:38Z

packages/pwa-kit-dev/src/configs/webpack/config.js

@@ -498,8 +498,13 @@ const ssr = (() => {
    if (mode === production) {
        return baseConfig('node')
            .extend((config) => {
+                let additionalConfig = {}
+                if (process.env.SSR_SOURCE_MAP === 'true') {


Suggested change

if (process.env.SSR_SOURCE_MAP === 'true') {

if (process.env.PWA_KIT_SSR_SOURCE_MAP === 'true') {

Namespacing env vars is good/consistent 🙂

Co-authored-by: Will Harney <[email protected]> Signed-off-by: Yuna Kim <[email protected]>

bfeister

Will's point about namespacing is not wrong, but not a blocker

yunakim714 · 2023-11-30T16:22:06Z

@wjhsf @bfeister Should I add push:pwa-kit-ssr-source-map to this line here in smoke-test-npm-scripts.js?

wjhsf · 2023-11-30T21:41:35Z

packages/template-retail-react-app/package.json

@@ -20,6 +20,7 @@
    "lint:fix": "npm run lint -- --fix",
    "postinstall": "npm run compile-translations && npm run compile-translations:pseudo",
    "push": "npm run build && pwa-kit-dev push",
+    "push:ssr-source-map": "cross-env PWA_KIT_SSR_SOURCE_MAP=true npm run build && pwa-kit-dev push",


Suggested change

"push:ssr-source-map": "cross-env PWA_KIT_SSR_SOURCE_MAP=true npm run build && pwa-kit-dev push",

"push:ssr-source-map": "cross-env PWA_KIT_SSR_SOURCE_MAP=true npm run push",

This actually makes me wonder, though, what is the point of having a specific npm script for this? Why not just document the feature and let people set the environment variable when they want to use it?

Another idea: We could add a flag to pwa-kit-dev build, copying the pattern for the buildDirectory flag.

agreed with Will on point of having this script.

@wjhsf @alexvuong Will get back to this once I have approval on a new tech proposal from the platform MRT team.

yunakim714 · 2023-12-19T16:43:15Z

@bfeister @alexvuong @wjhsf New implementation of enabling source maps will be merged with this PR. Users will be able to toggle the enable_source_maps field through the existing Target API's. When enable_source_maps is set to true, users will see an improved error stack in their logs if their bundle is uploaded with source maps.

Based on this implementation, I agree that we do not really need the script, but just documentation on the env var used in pwa-kit-dev to output the ssr.js.map. Wanted to get yalls feedback on this. Thank you!

…ud/pwa-kit into enable-source-maps

bfeister

Minor caveat: we should have a documentation update that exposes this to customers, but not a blocker for this PR

hajinsuha1

🚢 🚢 🚢

yunakim714 added 3 commits November 27, 2023 11:54

set devtool to sourcemap

64ed55e

add script for ssr source map

8d17124

add script to package

077520b

bredmond-sf self-requested a review November 28, 2023 19:27

bredmond-sf reviewed Nov 28, 2023

View reviewed changes

yunakim714 requested a review from hajinsuha1 November 28, 2023 21:36

change script to ssr-source-map to be more specific

5507e89

yunakim714 requested a review from bfeister November 29, 2023 20:06

bfeister reviewed Nov 29, 2023

View reviewed changes

packages/template-retail-react-app/package.json Outdated Show resolved Hide resolved

wjhsf reviewed Nov 29, 2023

View reviewed changes

yunakim714 and others added 2 commits November 30, 2023 10:49

Inline spread devtool property

c1828bf

Co-authored-by: Will Harney <[email protected]> Signed-off-by: Yuna Kim <[email protected]>

Merge branch 'develop' into enable-source-maps

62ce5b8

yunakim714 marked this pull request as ready for review November 30, 2023 15:49

yunakim714 requested a review from a team as a code owner November 30, 2023 15:49

bfeister previously approved these changes Nov 30, 2023

View reviewed changes

cleanup and prevent ssr-source-map from running in ci

16ddba7

yunakim714 dismissed bfeister’s stale review via 16ddba7 November 30, 2023 19:12

namespace env var PWA_KIT_SSR_SOURCE_MAP

90793d4

bfeister previously approved these changes Nov 30, 2023

View reviewed changes

wjhsf reviewed Nov 30, 2023

View reviewed changes

Merge branch 'develop' into enable-source-maps

c6b99c9

yunakim714 and others added 3 commits December 19, 2023 11:43

Merge branch 'develop' into enable-source-maps

1a0b712

remove script

d447b51

Merge branch 'enable-source-maps' of github.com:SalesforceCommerceClo…

a8dbce6

…ud/pwa-kit into enable-source-maps

yunakim714 dismissed bfeister’s stale review via a8dbce6 December 28, 2023 15:47

Merge branch 'develop' into enable-source-maps

b484619

yunakim714 changed the title ~~Generate SSR source map with script npm run push:source-map~~ Generate SSR source map with environment variable Jan 3, 2024

bfeister approved these changes Jan 3, 2024

View reviewed changes

yunakim714 requested review from hajinsuha1 and removed request for hajinsuha1 January 3, 2024 18:42

hajinsuha1 approved these changes Jan 3, 2024

View reviewed changes

yunakim714 merged commit 34ae8b5 into develop Jan 3, 2024
28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate SSR source map with environment variable #1571

Generate SSR source map with environment variable #1571

yunakim714 commented Nov 28, 2023 •

edited

Loading

bredmond-sf Nov 28, 2023

bfeister Nov 29, 2023

wjhsf Nov 29, 2023

bfeister left a comment

yunakim714 commented Nov 30, 2023

wjhsf Nov 30, 2023

alexvuong Dec 4, 2023

yunakim714 Dec 15, 2023

yunakim714 commented Dec 19, 2023

bfeister left a comment

hajinsuha1 left a comment

	if (process.env.SSR_SOURCE_MAP === 'true') {
	if (process.env.PWA_KIT_SSR_SOURCE_MAP === 'true') {

	"push:ssr-source-map": "cross-env PWA_KIT_SSR_SOURCE_MAP=true npm run build && pwa-kit-dev push",
	"push:ssr-source-map": "cross-env PWA_KIT_SSR_SOURCE_MAP=true npm run push",

Generate SSR source map with environment variable #1571

Generate SSR source map with environment variable #1571

Conversation

yunakim714 commented Nov 28, 2023 • edited Loading

Description

Types of Changes

Changes

How to Test-Drive This PR

Checklists

General

Accessibility Compliance

Localization

bredmond-sf Nov 28, 2023

Choose a reason for hiding this comment

bfeister Nov 29, 2023

Choose a reason for hiding this comment

wjhsf Nov 29, 2023

Choose a reason for hiding this comment

bfeister left a comment

Choose a reason for hiding this comment

yunakim714 commented Nov 30, 2023

wjhsf Nov 30, 2023

Choose a reason for hiding this comment

alexvuong Dec 4, 2023

Choose a reason for hiding this comment

yunakim714 Dec 15, 2023

Choose a reason for hiding this comment

yunakim714 commented Dec 19, 2023

bfeister left a comment

Choose a reason for hiding this comment

hajinsuha1 left a comment

Choose a reason for hiding this comment

yunakim714 commented Nov 28, 2023 •

edited

Loading