Skip to content

Conversation

dudiq
Copy link

@dudiq dudiq commented May 24, 2022

In some cases tsconfig.json may extends from other file. And it may have baseUrl with paths fields inside

@zachbryant zachbryant self-assigned this May 24, 2022
@zachbryant zachbryant added the enhancement New feature or request label May 24, 2022
@zachbryant
Copy link
Owner

@dudiq is this PR still alive?

@dudiq
Copy link
Author

dudiq commented Jun 10, 2022

@zachbryant i guess this question is more to you. i can't merge it to master and resolve problem with extend field

@zachbryant
Copy link
Owner

@dudiq are you able to see the comments I left in review?

@dudiq
Copy link
Author

dudiq commented Jun 10, 2022

@zachbryant nope

@dudiq
Copy link
Author

dudiq commented Jun 10, 2022

@zachbryant still no any comments

);
if (!result?.config) {
throw new Error(`Missing or invalid tsconfig.json in project root (${options.projectRoot})`);
throw new Error(`Missing or invalid ${names.join(',')} in project root (${options.projectRoot})`);
Copy link
Owner

Choose a reason for hiding this comment

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

nit: add space after comma

}

async function loadCompilerOptions(options, resolveFrom) {
let baseConfig = await loadConfigByName(['tsconfig.json', 'tsconfig.js'], options, resolveFrom)
Copy link
Owner

Choose a reason for hiding this comment

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

The naming of this variable is ambiguous. I suggest childConfig for this line and parentConfig in place of extendsConfig

return result.config;
}

async function loadCompilerOptions(options, resolveFrom) {
Copy link
Owner

Choose a reason for hiding this comment

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

To reduce references to compilerOptions let's keep loadConfig and have loadCompilerOptions call the former and do error checks

return baseConfig.compilerOptions
}
let extendsConfig = await loadConfigByName([baseConfig.extends], options, resolveFrom)
return extendsConfig?.compilerOptions || baseConfig?.compilerOptions ? {
Copy link
Owner

Choose a reason for hiding this comment

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

If the project tsconfig extends another, this line drops the base config entirely. Unless items are undefined (I expect missing keys to just be missing rather than undefined, but I'll double check), it should just be:

return {
  ...extendsConfig.compilerOptions,
  ...baseConfig.compilerOptions,
};

return baseConfig.compilerOptions
}
let extendsConfig = await loadConfigByName([baseConfig.extends], options, resolveFrom)
return extendsConfig?.compilerOptions || baseConfig?.compilerOptions ? {
Copy link
Owner

Choose a reason for hiding this comment

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

loadConfigByName checks for null results already, so qualifiers are not needed

/** Populate a map with any paths from tsconfig.json starting from baseUrl */
async function loadTsPaths(resolveFrom: string, options, logger): Promise<PathMapType> {
let config = await loadConfig(options, resolveFrom);
let compilerOptions = config?.['compilerOptions'];
Copy link
Owner

Choose a reason for hiding this comment

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

I would like to keep this line just to reduce code size

@zachbryant
Copy link
Owner

@zachbryant still no any comments

Woops, I don't use github for reviews enough 😂 should be visible now

@jlalmes
Copy link

jlalmes commented Nov 3, 2022

Hi @dudiq & @zachbryant, any updates on this? I'm happy to open a new PR if this is no longer being worked on.

@zachbryant
Copy link
Owner

Hey @jlalmes , I'd be happy to see a PR here. I can get to this shortly if you'd prefer I do it. Timelines on updates for this have slowed as I don't use parcel much these days

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants