fix: env refactor for Vite wrap-up [LIBS-690] (#889)

* fix: prefer process.env for env vars

* fix: remove undefined values from env

* refactor: make PWA env vars more resilient to undefineds

* docs: make sure PWA config options are well-documented

* refactor: remove verbose pwa defaults

* chore: comments

* chore: yarn lock

cli/config/d2ConfigDefaults.js

@@ -22,55 +22,4 @@ const defaultsLib = {
     },
 }
 
-const defaultsPWA = {
-    pwa: {
-        /**
-         * If true, service worker is registered to perform offline caching
-         * and use of cacheable sections & recording mode is enabled
-         */
-        enabled: false,
-        caching: {
-            /**
-             * If true, don't cache requests to exteral domains in app shell.
-             * Doesn't affect recording mode
-             */
-            omitExternalRequestsFromAppShell: false,
-            /** Deprecated version of above */
-            omitExternalRequests: false,
-            /**
-             * Don't cache URLs matching patterns in this array in app shell.
-             * Doesn't affect recording mode
-             */
-            patternsToOmitFromAppShell: [],
-            /** Deprecated version of above */
-            patternsToOmit: [],
-            /**
-             * Don't cache URLs matching these patterns in recorded sections.
-             * Can still be cached in app shell unless filtered there too.
-             */
-            patternsToOmitFromCacheableSections: [],
-            /**
-             * In addition to the contents of an app's 'build' folder, other
-             * URLs can be precached by adding them to this list which will
-             * add them to the precache manifest at build time. The format of
-             * this list must match the Workbox precache list format:
-             * https://developers.google.com/web/tools/workbox/modules/workbox-precaching#explanation_of_the_precache_list
-             */
-            additionalManifestEntries: [],
-            /**
-             * By default, all the contents of the `build` folder are added to
-             * the precache to give the app the best chances of functioning
-             * completely while offline. Developers may choose to omit some
-             * of these files (for example, thousands of font or image files)
-             * if they cause cache bloat and the app can work fine without
-             * them precached. See LIBS-482
-             *
-             * The globs should be relative to the public dir of the built app.
-             * Used in injectPrecacheManifest.js
-             */
-            globsToOmitFromPrecache: [],
-        },
-    },
-}
-
-module.exports = { defaultsApp, defaultsLib, defaultsPWA }
+module.exports = { defaultsApp, defaultsLib }

cli/config/d2ConfigDefaults.typescript.js

@@ -14,55 +14,4 @@ const defaultsLib = {
     },
 }
 
-const defaultsPWA = {
-    pwa: {
-        /**
-         * If true, service worker is registered to perform offline caching
-         * and use of cacheable sections & recording mode is enabled
-         */
-        enabled: false,
-        caching: {
-            /**
-             * If true, don't cache requests to exteral domains in app shell.
-             * Doesn't affect recording mode
-             */
-            omitExternalRequestsFromAppShell: false,
-            /** Deprecated version of above */
-            omitExternalRequests: false,
-            /**
-             * Don't cache URLs matching patterns in this array in app shell.
-             * Doesn't affect recording mode
-             */
-            patternsToOmitFromAppShell: [],
-            /** Deprecated version of above */
-            patternsToOmit: [],
-            /**
-             * Don't cache URLs matching these patterns in recorded sections.
-             * Can still be cached in app shell unless filtered there too.
-             */
-            patternsToOmitFromCacheableSections: [],
-            /**
-             * In addition to the contents of an app's 'build' folder, other
-             * URLs can be precached by adding them to this list which will
-             * add them to the precache manifest at build time. The format of
-             * this list must match the Workbox precache list format:
-             * https://developers.google.com/web/tools/workbox/modules/workbox-precaching#explanation_of_the_precache_list
-             */
-            additionalManifestEntries: [],
-            /**
-             * By default, all the contents of the `build` folder are added to
-             * the precache to give the app the best chances of functioning
-             * completely while offline. Developers may choose to omit some
-             * of these files (for example, thousands of font or image files)
-             * if they cause cache bloat and the app can work fine without
-             * them precached. See LIBS-482
-             *
-             * The globs should be relative to the public dir of the built app.
-             * Used in injectPrecacheManifest.js
-             */
-            globsToOmitFromPrecache: [],
-        },
-    },
-}
-
-module.exports = { defaultsApp, defaultsLib, defaultsPWA }
+module.exports = { defaultsApp, defaultsLib }

cli/config/makeViteConfig.mjs

@@ -18,7 +18,7 @@ import dynamicImport from 'vite-plugin-dynamic-import'
  * Vite normally throws an error when JSX syntax is used in a file without a
  * .jsx or .tsx extension. This is by design, in order to improve transform
  * performance by not parsing JS files for JSX.
- * 
+ *
  * This plugin and the `optimizeDeps` options in this config object,
  * along with the `jsxRuntime: 'classic'` option in the React plugin of the main
  * config, can enable support for JSX in .js files. This config object is
@@ -28,20 +28,22 @@ import dynamicImport from 'vite-plugin-dynamic-import'
  * todo: deprecate -- this config has a performance cost, especially on startup
  */
 const jsxInJsConfig = {
-    plugins: [{
-        name: 'treat-js-files-as-jsx',
-        async transform(code, id) {
-            if (!id.match(/src\/.*\.js$/)) {
-                return null
-            }
-            // Use the exposed transform from vite, instead of directly
-            // transforming with esbuild
-            return transformWithEsbuild(code, id, {
-                loader: 'jsx',
-                jsx: 'automatic',
-            })
+    plugins: [
+        {
+            name: 'treat-js-files-as-jsx',
+            async transform(code, id) {
+                if (!id.match(/src\/.*\.js$/)) {
+                    return null
+                }
+                // Use the exposed transform from vite, instead of directly
+                // transforming with esbuild
+                return transformWithEsbuild(code, id, {
+                    loader: 'jsx',
+                    jsx: 'automatic',
+                })
+            },
         },
-    }],
+    ],
     optimizeDeps: {
         force: true,
         esbuildOptions: { loader: { '.js': 'jsx' } },
@@ -72,8 +74,17 @@ const handleAssetFileNames = ({ name }) => {
 
 /**
  * Setting up static variable replacements at build time.
- * Use individual properties for drop-in replacements instead of a whole
+ * Vite adds env vars (from .env files, user env, and CLI args) to
+ * `import.meta.env`; for backwards compatibility and generalization, we also
+ * add those to `process.env`.
+ *
+ * Note that variables added to `import.meta.env` here will be available in
+ * index.html, e.g. import.meta.env.DHIS2_APP_NAME will populate
+ * %DHIS2_APP_NAME% in HTML
+ *
+ * Uses individual properties for drop-in replacements instead of a whole
  * object, which allows for better dead code elimination.
+ *
  * For env vars for now, we keep the behavior in /src/lib/shell/env.js:
  * loading, filtering, and prefixing env vars for CRA.
  * Once we remove support for those variables, we just need:
@@ -86,17 +97,16 @@ const handleAssetFileNames = ({ name }) => {
 const getDefineOptions = (env) => {
     const defineOptions = {}
     Object.entries(env).forEach(([key, val]) => {
-        // 'DHIS2_'-prefixed vars go on import.meta.env
+        // Each `val` should be a string already, but we need to stringify again
+        // for it to appear as a string in the resulting code:
+        // '"value"' here => 'value' in the code
+        const stringifiedVal = JSON.stringify(val)
+
+        defineOptions[`process.env.${key}`] = stringifiedVal
+        // 'DHIS2_'-prefixed vars go on import.meta.env too
         if (key.startsWith('DHIS2_')) {
-            defineOptions[`import.meta.env.${key}`] = JSON.stringify(val)
-            return
+            defineOptions[`import.meta.env.${key}`] = stringifiedVal
         }
-        // For backwards compatibility, add REACT_APP_DHIS2_... and other env
-        // vars to process.env. These env vars have been filtered by getEnv().
-        // They will be statically replaced at build time.
-        // Env vars in this format will be removed in future versions
-        // todo: deprecate in favor of import.meta.env
-        defineOptions[`process.env.${key}`] = JSON.stringify(val)
     })
     return defineOptions
 }
@@ -169,7 +179,7 @@ export default ({ paths, config, env, host, force, allowJsxInJs }) => {
             react({
                 babel: { plugins: ['styled-jsx/babel'] },
                 // todo: deprecate with other jsx-in-js config
-                // This option allows HMR of JSX-in-JS files, 
+                // This option allows HMR of JSX-in-JS files,
                 // but it isn't possible to add with merge config:
                 jsxRuntime: allowJsxInJs ? 'classic' : 'automatic',
             }),

cli/src/commands/build.js

@@ -154,7 +154,7 @@ const handler = async ({
                 })
                 await build(viteConfig)
 
-                if (config.pwa.enabled) {
+                if (config.pwa?.enabled) {
                     reporter.info('Compiling service worker...')
                     await compileServiceWorker({ env, paths, mode })
 

cli/src/commands/start.js

@@ -126,7 +126,7 @@ const handler = async ({
 
             const env = getEnv({ config, publicUrl: '.' })
 
-            if (config.pwa.enabled) {
+            if (config.pwa?.enabled) {
                 reporter.info('Compiling service worker...')
                 await compileServiceWorker({ env, paths, mode })
                 // don't need to inject precache manifest because no precaching

cli/src/lib/env/getEnv.js

@@ -29,55 +29,68 @@ const prefixEnvForCRA = (env) =>
         {}
     )
 
+/** Set up variables relevant to the App Shell */
 const getShellEnv = (config) => {
     const shellEnv = {
         name: config.title,
         version: config.version,
         loginApp: config.type === 'login_app' ? 'true' : undefined,
         direction: config.direction,
-        // NB: 'IS_PLUGIN' is added by string replacement in
-        // compiler/entrypoints.js, since env is shared between app and plugin
         requiredProps: config.requiredProps?.join(),
         skipPluginLogic: config.skipPluginLogic ? 'true' : undefined,
         ...getPWAEnvVars(config),
+        // NB: 'IS_PLUGIN' is added by string replacement in
+        // compiler/entrypoints.js, since env is shared between app and plugin
     }
 
-    // Remove undefined values and prefix with DHIS2_APP_
-    const filteredAndPrefixedShellEnv = Object.entries(shellEnv).reduce(
+    // Prefix with DHIS2_APP_
+    const prefixedShellEnv = Object.entries(shellEnv).reduce(
         (newEnv, [key, value]) => {
-            if (typeof value === 'undefined') {
-                return newEnv
-            }
             return {
                 ...newEnv,
                 [`DHIS2_APP_${key.toUpperCase()}`]: value,
             }
         },
         {}
     )
-    return filteredAndPrefixedShellEnv
+    return prefixedShellEnv
+}
+
+/**
+ * 1. Removes keys with `undefined` values to avoid noise
+ * 2. Double-checks to make sure all values are strings
+ */
+const cleanEntries = (env) => {
+    return Object.entries(env).reduce((newEnv, [key, value]) => {
+        if (value === undefined) {
+            return newEnv
+        }
+        return {
+            ...newEnv,
+            [key]: typeof value === 'string' ? value : JSON.stringify(value),
+        }
+    }, {})
 }
 
 module.exports = ({ config, baseUrl, publicUrl }) => {
     const filteredEnv = filterEnv()
     const shellEnv = getShellEnv(config)
     const DHIS2_BASE_URL = baseUrl
 
-    const env = {
-        // Legacy env vars; deprecated
+    const env = cleanEntries({
+        // Legacy env var prefix; deprecated
         ...prefixEnvForCRA({
             DHIS2_BASE_URL,
             ...filteredEnv,
             ...shellEnv,
         }),
-        // New form for env vars: import.meta.env.DHIS2_etc
+        // New keys for env vars: process.env.DHIS2_etc
         ...filteredEnv,
         ...shellEnv,
-        NODE_ENV: process.env.NODE_ENV,
         DHIS2_BASE_URL,
-        // todo: deprecated; migrate to import.meta.env.BASE_URL
+        NODE_ENV: process.env.NODE_ENV,
         PUBLIC_URL: publicUrl || '.',
-    }
+    })
 
     if (env.REACT_APP_DHIS2_API_VERSION) {
         reporter.warn(

cli/src/lib/env/getPWAEnvVars.js

@@ -12,6 +12,9 @@ function escapeForRegex(string) {
  * @param {Object} config
  */
 function stringifyPatterns(patternsList) {
+    if (patternsList === undefined) {
+        return undefined
+    }
     const stringsList = patternsList.map((pattern) => {
         if (typeof pattern === 'string') {
             return escapeForRegex(pattern)
@@ -36,29 +39,31 @@ function getPWAEnvVars(config) {
     if (!isApp(config.type)) {
         return null
     }
-    if (!config.pwa.enabled) {
+    if (!config.pwa?.enabled) {
         // Explicitly adding this value to the env helps pare down code in
         // non-PWA apps when doing static bundle analysis
         return { pwa_enabled: 'false' }
     }
     return {
-        pwa_enabled: JSON.stringify(config.pwa.enabled),
-        pwa_caching_omit_external_requests_from_app_shell: JSON.stringify(
-            config.pwa.caching.omitExternalRequestsFromAppShell
-        ),
+        pwa_enabled: 'true',
+        pwa_caching_omit_external_requests_from_app_shell: config.pwa?.caching
+            ?.omitExternalRequestsFromAppShell
+            ? 'true'
+            : undefined,
         // Deprecated version of the above:
-        pwa_caching_omit_external_requests: JSON.stringify(
-            config.pwa.caching.omitExternalRequests
-        ),
+        pwa_caching_omit_external_requests: config.pwa?.caching
+            ?.omitExternalRequests
+            ? 'true'
+            : undefined,
         pwa_caching_patterns_to_omit_from_app_shell: stringifyPatterns(
-            config.pwa.caching.patternsToOmitFromAppShell
+            config.pwa?.caching?.patternsToOmitFromAppShell
         ),
         // Deprecated version of the above:
         pwa_caching_patterns_to_omit: stringifyPatterns(
-            config.pwa.caching.patternsToOmit
+            config.pwa?.caching?.patternsToOmit
         ),
         pwa_caching_patterns_to_omit_from_cacheable_sections: stringifyPatterns(
-            config.pwa.caching.patternsToOmitFromCacheableSections
+            config.pwa?.caching?.patternsToOmitFromCacheableSections
         ),
     }
 }