Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix getConfig referencing config from incorrect location #1049

Merged
merged 8 commits into from
Mar 13, 2023
Merged

Fix getConfig referencing config from incorrect location #1049

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3696e80
Initial Commit
bendvc Mar 10, 2023
1746460
Update CHANGELOG.md
bendvc Mar 10, 2023
c2c44ff
Revert some testing code
bendvc Mar 10, 2023
57087bb
Merge branch 'develop' into fix-get-config
bendvc Mar 13, 2023
08c410d
Merge branch 'develop' into fix-get-config
bendvc Mar 13, 2023
242242d
Get test coverage back up
bendvc Mar 13, 2023
5a0928c
Merge branch 'develop' into fix-get-config
bendvc Mar 13, 2023
5e18a6f
Test build directory folder before proceeding.
bendvc Mar 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions packages/pwa-kit-dev/bin/pwa-kit-dev.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ const main = async () => {
.addOption(
new program.Option(
'-b, --buildDirectory <buildDirectory>',
'a custom project directory where your build is located'
'a custom project build directory that you want to push'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I feel like this description was a little misleading, and clarified that it's the build folder that we want.

).default(p.join(process.cwd(), 'build'), './build')
)
.addOption(
Expand Down Expand Up @@ -285,12 +285,20 @@ const main = async () => {
credentialsFile
}) => {
// Set the deployment target env var, this is required to ensure we
// get the correct configuration object.
process.env.DEPLOY_TARGET = target
// get the correct configuration object. Do not assign the variable it if
// the target value is `undefined` as it will serialied as a "undefined"
// string value.
if (target) {
process.env.DEPLOY_TARGET = target
}
Comment on lines +291 to +293
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I noticed that when this environment variable is used in the ssr-config.js file, its value was always defined. In fact it was a string literal "undefined", this meant that the locations were cosmiconfig searched included config/undefined.ext meaning you could drop a file named "undefined.json" in your config and it would try reading it.

Adding a guard fixed this.


const credentials = await scriptUtils.readCredentials(credentialsFile)

const mobify = getConfig() || {}
if (!fse.pathExistsSync(buildDirectory)) {
throw new Error(`Supplied "buildDirectory" does not exist!`)
}

const mobify = getConfig({buildDirectory}) || {}

if (!projectSlug) {
projectSlug = await getProjectName()
Expand Down
1 change: 1 addition & 0 deletions packages/pwa-kit-runtime/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
## v2.8.0-dev (Mar 03, 2023)
- Add optional parameter to override configuration folder used in `getConfig` [#1049](https://github.com/SalesforceCommerceCloud/pwa-kit/pull/1049)
## v2.7.0 (Mar 03, 2023)
- Support Node 16 [#965](https://github.com/SalesforceCommerceCloud/pwa-kit/pull/965)

Expand Down
31 changes: 31 additions & 0 deletions packages/pwa-kit-runtime/src/utils/ssr-config.client.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/*
* Copyright (c) 2023, Salesforce, Inc.
* All rights reserved.
* SPDX-License-Identifier: BSD-3-Clause
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/BSD-3-Clause
*/

/* eslint-env jest */
/* eslint max-nested-callbacks:0 */

import {getConfig} from './ssr-config.client'

let windowSpy

beforeEach(() => {
windowSpy = jest.spyOn(window, 'window', 'get')
})

afterEach(() => {
windowSpy.mockRestore()
})

describe('Client getConfig', () => {
test('returns window.__CONFIG__ value', () => {
windowSpy.mockImplementation(() => ({
__CONFIG__: {}
}))

expect(getConfig()).toEqual({})
})
})
19 changes: 13 additions & 6 deletions packages/pwa-kit-runtime/src/utils/ssr-config.server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
/* istanbul ignore next */
const SUPPORTED_FILE_TYPES = ['js', 'yml', 'yaml', 'json']

const IS_REMOTE = Object.prototype.hasOwnProperty.call(process.env, 'AWS_LAMBDA_FUNCTION_NAME')

/**
* Returns the express app configuration file in object form. The file will be resolved in the
* the following order:
Expand All @@ -28,21 +30,26 @@ const SUPPORTED_FILE_TYPES = ['js', 'yml', 'yaml', 'json']
* Each file marked with `ext` can optionally be terminated with `js`, `yml|yaml` or
* `json`. The file loaded is also determined based on that precidence of file extension.
*
* @param {Object} opts.buildDirectory - Option path to the folder containing the configution. Byt default
* it is the `build` folder when running remotely and the project folder when developing locally.
* @returns - the application configuration object.
*/
/* istanbul ignore next */
export const getConfig = () => {
const isRemote = Object.prototype.hasOwnProperty.call(process.env, 'AWS_LAMBDA_FUNCTION_NAME')
export const getConfig = (opts = {}) => {
const {buildDirectory} = opts
const configDirBase = IS_REMOTE ? 'build' : ''
let targetName = process?.env?.DEPLOY_TARGET || ''

const targetSearchPlaces = SUPPORTED_FILE_TYPES.map((ext) => `config/${targetName}.${ext}`)
const localeSearchPlaces = SUPPORTED_FILE_TYPES.map((ext) => `config/local.${ext}`)
const defaultSearchPlaces = SUPPORTED_FILE_TYPES.map((ext) => `config/default.${ext}`)

const searchFrom = buildDirectory || process.cwd() + '/' + configDirBase
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 what's the value for buildDirectory here? Do we need to prepend it with process.cwd() perhaps?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The the idea here was to have the buildDirectory option in the 'build' command be symmetrical with the buildDirectory option in the 'push' commend. So that being said, it can accept both a relative path and an absolute path.

The weird thing I just spotted after you brought this up was that cosmiconfig didn't really care if the buildDirectory existed or not, because by default it will start traversing the file tree upwards looking for configuration files.

This behaviour in the context of pushing wasn't great because it would take a while to fail since it's possible that it found a package.json file further up the file tree and used that for the push config, then only failed during the build step.

So I took extra precaution and decided to throw if the buildDirectory you supply doesn't exist.

So after all that, to answer you question we are taking the absolute or relative path and passing it to cosmiconfig, which from the documentation doesn't specify if it has to be relative or absolute. It's possible that it resolves it internally.


// Combined search places.
const searchPlaces = [
...targetSearchPlaces,
...(!isRemote ? localeSearchPlaces : []),
...(targetName ? targetSearchPlaces : []),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

targetName is potentially undefined as mentioned in a previous comment. This condition ensures we aren't searching for files that we shouldn't. This condition could be places above, but I put it here to go with out the other condition below it works.

...(!IS_REMOTE ? localeSearchPlaces : []),
...defaultSearchPlaces,
'package.json'
]
Expand All @@ -52,7 +59,7 @@ export const getConfig = () => {
// Match config files based on the specificity from most to most general.
const explorerSync = cosmiconfigSync(targetName, {
packageProp: 'mobify',
searchPlaces: searchPlaces.map((path) => (isRemote ? `build/${path}` : path)),
searchPlaces: searchPlaces,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This logic is not required anymore as the same result is accomplished by using the searchFrom property in the search function below. (this option was also required to fix the original issue and look in a different folder).

loaders: {
'.js': (filepath) => {
// Because `require` is bootstrapped by webpack, the builtin
Expand All @@ -67,7 +74,7 @@ export const getConfig = () => {
})

// Load the config synchronously using a custom "searchPlaces".
const {config} = explorerSync.search() || {}
const {config} = explorerSync.search(searchFrom) || {}

if (!config) {
throw new Error(
Expand Down
33 changes: 33 additions & 0 deletions packages/pwa-kit-runtime/src/utils/ssr-config.server.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/*
* Copyright (c) 2023, Salesforce, Inc.
* All rights reserved.
* SPDX-License-Identifier: BSD-3-Clause
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/BSD-3-Clause
*/

/* eslint-env jest */
/* eslint max-nested-callbacks:0 */

import {getConfig} from './ssr-config.server'

describe('Server "getConfig"', () => {
const env = process.env

beforeEach(() => {
jest.resetModules()
process.env = {...env}
})

afterEach(() => {
process.env = env
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: Should this be the same as on line 18 above?

Copy link
Collaborator Author

@bendvc bendvc Mar 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I essentially borrowed that from here. I believe the idea is to restore to the actual value and not a copy of the original values. And to also only modify a copy, not modify (add/remove) values during the rest run.

})

test('throws when no config files are found running remotely', () => {
expect(getConfig).toThrow()
})

test('throws when no config files are found running locally', () => {
process.env.AWS_LAMBDA_FUNCTION_NAME = ''
expect(getConfig).toThrow()
})
})