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

Jump to bottom

Fix output path in build task #334

Merged
merged 8 commits into from
Sep 23, 2024
Merged

Fix output path in build task #334

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
2a4019d
Avoid duplicates in --help
daogrady Sep 19, 2024
63dbbb2
Properly configure outputDirectory in build task
daogrady Sep 19, 2024
f1e70cf
Merge branch 'main' into fix/build-task-output
daogrady Sep 19, 2024
090409a
Bail early if outputDirectory is found in cds.env
daogrady Sep 19, 2024
c0ba091
Fix doc type
daogrady Sep 23, 2024
452155d
Forgo stateless approach
daogrady Sep 23, 2024
44d5713
Load configuration lazily
daogrady Sep 23, 2024
cf576d0
Properly initialise configs when cloning
daogrady Sep 23, 2024
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
13 changes: 10 additions & 3 deletions cds-plugin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ const rmDirIfExists = dir => {
* Remove files with given extensions from a directory recursively.
* @param {string} dir - The directory to start from.
* @param {string[]} exts - The extensions to remove.
* @returns {Promise<void>}
* @returns {Promise<unknown>}
*/
const rmFiles = async (dir, exts) => fs.existsSync(dir)
? Promise.all(
Expand Down Expand Up @@ -68,10 +68,15 @@ cds.build?.register?.('typescript', class extends cds.build.Plugin {

get #appFolder () { return cds?.env?.folders?.app ?? 'app' }

/**
* cds.env > tsconfig.compilerOptions.paths > '@cds-models' (default)
*/
get #modelDirectoryName () {
const outputDirectory = cds.env.typer?.outputDirectory
if (outputDirectory) return outputDirectory
try {
// expected format: { '#cds-models/*': [ './@cds-models/*/index.ts' ] }
// ^^^^^^^^^^^^^^^
// ^^^^^^^^^^^
// relevant part - may be changed by user
const config = JSON.parse(fs.readFileSync ('tsconfig.json', 'utf8'))
const alias = config.compilerOptions.paths['#cds-models/*'][0]
Expand All @@ -89,7 +94,9 @@ cds.build?.register?.('typescript', class extends cds.build.Plugin {

async #runCdsTyper () {
DEBUG?.('running cds-typer')
await typer.compileFromFile('*', { outputDirectory: this.#modelDirectoryName })
cds.env.typer ??= {}
cds.env.typer.outputDirectory ??= this.#modelDirectoryName
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not have the expected effect, due to the fact that the module ./lib/compile is implicitly loading module ./lib/config. So, the constructor of the Config class has already been executed and any changes to cds.env.typer will be ignored.

You would have to change it to

const { configuration } = require('@cap-js/cds-typer/lib/config')
...
configuration.outputDirectory ??= this.#modelDirectoryName

Copy link
Contributor Author

@daogrady daogrady Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

You're right, the properties should actually not be imprinted into the config at all, but dynamically queried from cds.env.
The joys of global state... I will check it again.

Copy link
Contributor Author

@daogrady daogrady Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

(the fix should actually work, as line 98 is basically a no-op when cds.env is present, since #modelDirectoryName will attempt to read the cds.typer.outputDirectory, but it is still bad style)

Copy link
Contributor

Choose a reason for hiding this comment

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

So,.... I did not mark it just because of code style 😊. I actually got an error during the build. I only had useEntitiesProxy in the env and the outputDirectory should be read via the default way - so from tsconfig.json.

This results in the same bug I already mentioned
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see your use case now and I am afraid you are correct.
I have made the initialisation of configuration lazy to support modifications of cds.env before the first actual use of cds-typer.

Copy link
Contributor

@stockbal stockbal Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Looks good. The build finishes without any errors now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great! Thanks for bringing this to our attention and for the review. 🙂

await typer.compileFromFile('*')
}

async #buildWithConfig () {
Expand Down
18 changes: 17 additions & 1 deletion lib/config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,17 +37,32 @@ class Config {
inlineDeclarations: 'flat'
}

constructor() {
values = undefined
proxy = undefined

init () {
this.values = {...Config.#defaults, ...(cds.env.typer ?? {})}
this.proxy = camelSnakeHybrid(this.values)
}

constructor() {
// proxy around config still allows arbitrary property access:
// require('config').configuration.logLevel = 'warn' will work
// eslint-disable-next-line no-constructor-return
return new Proxy(this, {
get(target, prop) {
// lazy loading of cds.env
// if we don't do this, configuration will load cds.env whenever it is
// first imported anywhere (even by proxy from, say, cli.js).
// So we don't get to modify cds.env before that, which is important
// in cds-build.js.
// FIXME: revisit. This is horrible.
if (target.values === undefined) target.init()
return target[prop] ?? target.proxy[prop]
},
set(target, p, v) {
if (target.values === undefined) target.init()

// this.value, this.proxy etc should not be forwarded to the wrapped values
if (target[p]) {
target[p] = v
Expand Down Expand Up @@ -88,6 +103,7 @@ class Config {

clone () {
const res = new Config()
res.init()
res.setMany(this.values)
return res
}
Expand Down
Loading