fix: base url env & refactor [LIBS-635] (#872)

* fix: missing base URL from env; refactor env and shell bootstrap

* fix: add public URL default

* fix: new env format for start script

* chore: clean up unused files

* fix: base url prefix

* refactor: parameter format

* chore: last comment

cli/config/makeViteConfig.mjs

@@ -78,18 +78,17 @@ const handleAssetFileNames = ({ name }) => {
  * and don't need to use `define`; they just need the envPrefix config.
  */
 const getDefineOptions = (env) => {
-    const defineOptions = {
-        'process.env.NODE_ENV': JSON.stringify(process.env.NODE_ENV),
-    }
+    const defineOptions = {}
     Object.entries(env).forEach(([key, val]) => {
         // 'DHIS2_'-prefixed vars go on import.meta.env
         if (key.startsWith('DHIS2_')) {
             defineOptions[`import.meta.env.${key}`] = JSON.stringify(val)
             return
         }
         // For backwards compatibility, add REACT_APP_DHIS2_... and other env
-        // vars to process.env. They will be statically replaced at build time.
-        // This will be removed in future versions
+        // 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)
     })
@@ -110,14 +109,15 @@ const getBuildInputs = (config, paths) => {
 // https://vitejs.dev/config/
 export default ({ paths, config, env, host }) => {
     return defineConfig({
-        // Need to specify the location of the app root, since we're not using
-        // the Vite CLI from the app root
+        // Need to specify the location of the app root, since this CLI command
+        // gets run in a different directory than the bootstrapped app
         root: paths.shell,
 
         // By default, assets are resolved to the root of the domain ('/'), but
         // deployed apps aren't served from there.
         // This option is basically the same as PUBLIC_URL for CRA and Parcel.
         // Works for both dev and production.
+        // Gets applied to import.meta.env.BASE_URL in the runtime code
         base: './',
 
         // Expose env vars with DHIS2_ prefix in index.html and on

cli/src/commands/build.js

@@ -1,16 +1,16 @@
 const path = require('path')
 const { reporter, chalk } = require('@dhis2/cli-helpers-engine')
 const fs = require('fs-extra')
+const bootstrapShell = require('../lib/bootstrapShell')
 const { compile } = require('../lib/compiler')
+const { loadEnvFiles, getEnv } = require('../lib/env')
 const exitOnCatch = require('../lib/exitOnCatch')
 const generateManifests = require('../lib/generateManifests')
 const i18n = require('../lib/i18n')
-const loadEnvFiles = require('../lib/loadEnvFiles')
 const parseConfig = require('../lib/parseConfig')
 const { isApp } = require('../lib/parseConfig')
 const makePaths = require('../lib/paths')
 const { injectPrecacheManifest, compileServiceWorker } = require('../lib/pwa')
-const makeShell = require('../lib/shell')
 const { validatePackage } = require('../lib/validatePackage')
 const { handler: pack } = require('./pack.js')
 
@@ -30,20 +30,22 @@ const getNodeEnv = () => {
 const printBuildParam = (key, value) => {
     reporter.print(chalk.green(` - ${key} :`), chalk.yellow(value))
 }
-const setAppParameters = (standalone, config) => {
-    process.env.PUBLIC_URL = process.env.PUBLIC_URL || '.'
-    printBuildParam('PUBLIC_URL', process.env.PUBLIC_URL)
+const getAppParameters = (standalone, config) => {
+    const publicUrl = process.env.PUBLIC_URL || '.'
+    printBuildParam('PUBLIC_URL', publicUrl)
 
     if (
         standalone === false ||
         (typeof standalone === 'undefined' && !config.standalone)
     ) {
         const defaultBase = config.coreApp ? `..` : `../../..`
-        process.env.DHIS2_BASE_URL = process.env.DHIS2_BASE_URL || defaultBase
+        const baseUrl = process.env.DHIS2_BASE_URL || defaultBase
 
-        printBuildParam('DHIS2_BASE_URL', process.env.DHIS2_BASE_URL)
+        printBuildParam('DHIS2_BASE_URL', baseUrl)
+        return { publicUrl, baseUrl }
     } else {
         printBuildParam('DHIS2_BASE_URL', '<standalone>')
+        return { publicUrl }
     }
 }
 
@@ -68,11 +70,9 @@ const handler = async ({
     printBuildParam('Mode', mode)
 
     const config = parseConfig(paths)
-    const shell = makeShell({ config, paths })
-
-    if (isApp(config.type)) {
-        setAppParameters(standalone, config)
-    }
+    const appParameters = isApp(config.type)
+        ? getAppParameters(standalone, config)
+        : null
 
     await fs.remove(paths.buildOutput)
 
@@ -107,7 +107,7 @@ const handler = async ({
 
             if (isApp(config.type)) {
                 reporter.info('Bootstrapping local appShell...')
-                await shell.bootstrap({ shell: shellSource, force })
+                await bootstrapShell({ paths, shell: shellSource, force })
             }
 
             reporter.info(
@@ -135,16 +135,13 @@ const handler = async ({
                 const { default: createConfig } = await import(
                     '../../config/makeViteConfig.mjs'
                 )
-                const viteConfig = createConfig({
-                    paths,
-                    config,
-                    env: shell.env,
-                })
+                const env = getEnv({ config, ...appParameters })
+                const viteConfig = createConfig({ paths, config, env })
                 await build(viteConfig)
 
                 if (config.pwa.enabled) {
                     reporter.info('Compiling service worker...')
-                    await compileServiceWorker({ config, paths, mode })
+                    await compileServiceWorker({ env, paths, mode })
 
                     reporter.info(
                         'Injecting supplementary precache manifest...'

cli/src/commands/start.js

@@ -1,16 +1,16 @@
 const { reporter, chalk } = require('@dhis2/cli-helpers-engine')
 const detectPort = require('detect-port')
+const bootstrapShell = require('../lib/bootstrapShell')
 const { compile } = require('../lib/compiler')
+const { loadEnvFiles, getEnv } = require('../lib/env')
 const exitOnCatch = require('../lib/exitOnCatch')
 const generateManifests = require('../lib/generateManifests')
 const i18n = require('../lib/i18n')
-const loadEnvFiles = require('../lib/loadEnvFiles')
 const parseConfig = require('../lib/parseConfig')
 const { isApp } = require('../lib/parseConfig')
 const makePaths = require('../lib/paths')
 const createProxyServer = require('../lib/proxy')
 const { compileServiceWorker } = require('../lib/pwa')
-const makeShell = require('../lib/shell')
 const { validatePackage } = require('../lib/validatePackage')
 
 const defaultPort = 3000
@@ -31,7 +31,6 @@ const handler = async ({
     loadEnvFiles(paths, mode)
 
     const config = parseConfig(paths)
-    const shell = makeShell({ config, paths })
 
     if (!isApp(config.type)) {
         reporter.error(
@@ -92,7 +91,7 @@ const handler = async ({
             })
 
             reporter.info('Bootstrapping local appShell...')
-            await shell.bootstrap({ shell: shellSource, force })
+            await bootstrapShell({ paths, shell: shellSource, force })
 
             reporter.info(`Building app ${chalk.bold(config.name)}...`)
             await compile({
@@ -113,9 +112,11 @@ const handler = async ({
                 )
             }
 
+            const env = getEnv({ config, publicUrl: '.' })
+
             if (config.pwa.enabled) {
                 reporter.info('Compiling service worker...')
-                await compileServiceWorker({ config, paths, mode })
+                await compileServiceWorker({ env, paths, mode })
                 // don't need to inject precache manifest because no precaching
                 // is done in development environments
             }
@@ -130,12 +131,7 @@ const handler = async ({
             const { default: createConfig } = await import(
                 '../../config/makeViteConfig.mjs'
             )
-            const viteConfig = createConfig({
-                config,
-                paths,
-                env: shell.env,
-                host,
-            })
+            const viteConfig = createConfig({ config, paths, env, host })
             const server = await createServer(viteConfig)
 
             const location = config.entryPoints.plugin

cli/src/commands/test.js

@@ -2,8 +2,8 @@ const path = require('path')
 const { reporter } = require('@dhis2/cli-helpers-engine')
 const { runCLI } = require('@jest/core')
 const fs = require('fs-extra')
+const { loadEnvFiles } = require('../lib/env')
 const exitOnCatch = require('../lib/exitOnCatch')
-const loadEnvFiles = require('../lib/loadEnvFiles')
 const makePaths = require('../lib/paths')
 
 const getAppJestConfig = ({ jestConfigPath, paths }) => {

cli/src/lib/shell/bootstrap.js → cli/src/lib/bootstrapShell.js

@@ -17,7 +17,7 @@ const getShellVersion = (shellDir) => {
     return '0'
 }
 
-const bootstrapShell = async (paths, { shell, force = false } = {}) => {
+const bootstrapShell = async ({ paths, shell, force = false }) => {
     const source = shell ? path.resolve(shell) : paths.shellSource,
         dest = paths.shell
 

cli/src/lib/env/getEnv.js

@@ -0,0 +1,93 @@
+const { reporter } = require('@dhis2/cli-helpers-engine')
+const getPWAEnvVars = require('./getPWAEnvVars')
+
+/**
+ * Filter process.env for just keys that start with DHIS2_
+ * to avoid leaking env
+ */
+const filterEnv = () =>
+    Object.keys(process.env)
+        .filter((key) => key.indexOf('DHIS2_') === 0)
+        .reduce(
+            (out, key) => ({
+                ...out,
+                [key]: process.env[key],
+            }),
+            {}
+        )
+
+/**
+ * Deprecated -- CRA did its own filtering;
+ * only env vars prefixed with REACT_APP_ would get passed to the app
+ */
+const prefixEnvForCRA = (env) =>
+    Object.keys(env).reduce(
+        (out, key) => ({
+            ...out,
+            [`REACT_APP_${key}`]: env[key],
+        }),
+        {}
+    )
+
+const getShellEnv = (config) => {
+    const shellEnv = {
+        name: config.title,
+        version: config.version,
+        loginApp: config.type === 'login_app' || 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,
+        ...getPWAEnvVars(config),
+    }
+
+    // Remove undefined values and prefix with DHIS2_APP_
+    const filteredAndPrefixedShellEnv = Object.entries(shellEnv).reduce(
+        (newEnv, [key, value]) => {
+            if (typeof value === 'undefined') {
+                return newEnv
+            }
+            return {
+                ...newEnv,
+                [`DHIS2_APP_${key.toUpperCase()}`]: value,
+            }
+        },
+        {}
+    )
+    return filteredAndPrefixedShellEnv
+}
+
+module.exports = ({ config, baseUrl, publicUrl }) => {
+    const filteredEnv = filterEnv()
+    const shellEnv = getShellEnv(config)
+    const DHIS2_BASE_URL = baseUrl
+
+    const env = {
+        // Legacy env vars; deprecated
+        ...prefixEnvForCRA({
+            DHIS2_BASE_URL,
+            ...filteredEnv,
+            ...shellEnv,
+        }),
+        // New form for env vars: import.meta.env.DHIS2_etc
+        ...filteredEnv,
+        ...shellEnv,
+        NODE_ENV: process.env.NODE_ENV,
+        DHIS2_BASE_URL,
+        // todo: deprecated; migrate to import.meta.env.BASE_URL
+        PUBLIC_URL: publicUrl || '.',
+    }
+
+    if (env.REACT_APP_DHIS2_API_VERSION) {
+        reporter.warn(
+            'Passing an explicit API version to the DHIS2 App Platform is not recommended.\n' +
+                'By default, the app platform will now use the latest API version available in the DHIS2 instance.\n' +
+                'Some API functionality may be unreliable when using an explicit API version.\n' +
+                'Support for the DHIS2_API_VERSION environment variable may be removed in a future release of the DHIS2 App Platform.'
+        )
+    }
+
+    reporter.debug('Env passed to app-shell:', env)
+    return env
+}

cli/src/lib/env/index.js

@@ -0,0 +1,7 @@
+const getEnv = require('./getEnv')
+const loadEnvFiles = require('./loadEnvFiles')
+
+module.exports = {
+    getEnv,
+    loadEnvFiles,
+}

cli/src/lib/pwa/compileServiceWorker.js

@@ -1,8 +1,6 @@
 const path = require('path')
 const { reporter } = require('@dhis2/cli-helpers-engine')
 const webpack = require('webpack')
-const getEnv = require('../shell/env')
-const getPWAEnvVars = require('./getPWAEnvVars')
 
 /**
  * Uses webpack to bundle a service worker. If used in development mode,
@@ -20,21 +18,14 @@ const getPWAEnvVars = require('./getPWAEnvVars')
  * @param {String} param0.mode - `'production'` or `'development'` (or other valid webpack `mode`s)
  * @returns {Promise}
  */
-function compileServiceWorker({ config, paths, mode }) {
+function compileServiceWorker({ env, paths, mode }) {
     // Choose appropriate destination for compiled SW based on 'mode'
     const isProduction = mode === 'production'
     const outputPath = isProduction
         ? paths.shellBuildServiceWorker
         : paths.shellPublicServiceWorker
     const { dir: outputDir, base: outputFilename } = path.parse(outputPath)
 
-    // This is part of a bit of a hacky way to provide the same env vars to dev
-    // SWs as in production by adding them to `process.env` using the plugin
-    // below.
-    // TODO: This could be refactored to be simpler now that we're not using
-    // CRA to build the service worker
-    const env = getEnv({ name: config.title, ...getPWAEnvVars(config) })
-
     const webpackConfig = {
         mode, // "production" or "development"
         devtool: isProduction ? false : 'source-map',
@@ -45,12 +36,8 @@ function compileServiceWorker({ config, paths, mode }) {
         },
         target: 'webworker',
         plugins: [
-            new webpack.DefinePlugin({
-                'process.env': JSON.stringify({
-                    ...env,
-                    NODE_ENV: process.env.NODE_ENV,
-                }),
-            }),
+            // Make sure SW has the same env vars as the app
+            new webpack.DefinePlugin({ 'process.env': JSON.stringify(env) }),
         ],
     }
 
@@ -81,7 +68,13 @@ function compileServiceWorker({ config, paths, mode }) {
                 return
             }
 
-            reporter.debug('Service Worker compilation successful')
+            reporter.debug(
+                'Service Worker compilation successful. Size:',
+                info.assets[0].size,
+                'bytes'
+            )
+            const outputPath = path.join(info.outputPath, info.assets[0].name)
+            reporter.debug('Output:', outputPath)
             resolve()
         })
     })

cli/src/lib/pwa/index.js

@@ -1,9 +1,7 @@
 const compileServiceWorker = require('./compileServiceWorker')
-const getPWAEnvVars = require('./getPWAEnvVars')
 const injectPrecacheManifest = require('./injectPrecacheManifest')
 
 module.exports = {
     compileServiceWorker,
-    getPWAEnvVars,
     injectPrecacheManifest,
 }