rokucommunity / brighterscript

A superset of Roku's BrightScript language
MIT License
160 stars 46 forks source link

[bsconfig] "stagingDir" property is not normalised to an absolute paths relative to the config file #1141

Closed bartvandenende-wm closed 3 months ago

bartvandenende-wm commented 5 months ago

Summary

"stagingDir" property in bsconfig.json is not resolved relative to the config file

Repro steps

given the below two config files

  1. ./node_modules/shared-config/bsconfig.json

    "rootDir": "../../src",
    "stagingDir": "../../build"
  2. ./bsconfig.json

    {
    "extends": "./node_modules/shared-config/bsconfig.json",
    }
  3. results in two different path resolutions

    import {Util} from "brighterscript"
    const util = new Util()
    const config = util.loadConfigFile("./bsconfig.json");
    console.log(`rootDir: ${config.rootDir}`)
    console.log(`stagingDir: ${config.stagingDir}`)

    Expected result:

$ rootDir: {cwd-dir}/src
$ stagingDir: {cwd-dir}/build

Actual result:

$ rootDir: {cwd}/src
$ stagingDir: {cwd}/../../build

Details

path normalisation is missing in the loadConfigFile utility for the stagingDir https://github.com/rokucommunity/brighterscript/blob/483a15454f1c8751619421ee4c99dbf4fdd073d7/src/util.ts#L227-L237

josephjunker commented 5 months ago

I wonder how much room there is for design here, if we consider breaking changes. The behavior that I would personally expect would be for the paths to be resolved relative to the child bsconfig. For example, if the desired result was

rootDir: {cwd}/src
stagingDir: {cwd}/build

then I would want ./node_modules/shared-config/bsconfig.json to contain this:

rootDir: "./src",
stagingDir: "./build"

Effectively the behavior would be to inline the paths from the parent config into the child config before resolving/normalizing them. I think this would lead to better decoupling. Thoughts @TwitchBronBron ?

Since this would be a breaking change it might be better to just make stagingDir behave the same as rootDir for now 🤔

TwitchBronBron commented 5 months ago

@josephjunker, I think I'd prefer to consistently resolve paths relative to their corresponding config location. I find that to be much easier to understand. It can become quite an import nightmare if the parent config paths were inlined into child configs at various different nested levels. Think about paths to plugins as one example.

@bartvandenende-wm, yeah, looks like stagingDir path resolution is wrong. It should be resolved in the same way as rootDir