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.

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 project argument to match tsc #709

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bendemboski
Copy link

The --project argument was being treated as a path to some location inside the project, rather than as either a path to a tsconfig file or a path to a folder containing a tsconfig file, which is how tsc implements it. This meant that there was no way to run glint and use a config file named anything other than tsconfig.json.

This is a breaking change because if anybody was passing a project path that pointed deeply into the project, that will no longer work. Similarly, the analyzeProject() unstable API's behavior has changed, and loadConfig export has been replaced with loadClosestConfig and loadConfigFromProject exports.

We could switch back to only having a single loadConfig() method that checks to see if its path points to a config file, and if not falls back on the pre-existing logic, but I thought that would be kinda confusing, and it would be better for the behavior to match that of tsc even though that requires a technically-breaking change.

Copy link
Member

@dfreeman dfreeman left a comment

Choose a reason for hiding this comment

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

Thank you for fixing this!

@@ -40,6 +41,6 @@ export function analyzeProject(projectDirectory: string = process.cwd()): Projec
};
}

export { loadConfig };
Copy link
Member

Choose a reason for hiding this comment

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

I know we've been careful not to make any commitments on the stability of any of these exports, but it looks like there are at least a couple real-world instances of folks relying on this.

Can we keep loadConfig around as a deprecated function that logs a warning that consumers should update to either loadClosestConfig or loadConfigFromProject and then calls through to loadClosestConfig to preserve existing behavior?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, makes sense! console.warn or do we have another method for issuing deprecation warnings?

*/
public configForProjectPath(configPath: string): GlintConfig | null {
let tsConfigPath = path.join(configPath, 'tsconfig.json');
let jsConfigPath = path.join(configPath, 'tsconfig.json');
Copy link
Member

Choose a reason for hiding this comment

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

Should this be jsconfig.json?

Copy link
Author

Choose a reason for hiding this comment

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

🤦 yes it should

The `--project` argument was being treated as a path to some location inside the project, rather than as either a path to a tsconfig file or a path to a folder containing a tsconfig file, which is how `tsc` implements it. This meant that there was no way to run `glint` and use a config file named anything other than `tsconfig.json`.

This is a breaking change because if anybody was passing a project path that pointed deeply into the project, that will no longer work. Similarly, the `analyzeProject()` unstable API's behavior has changed, and `loadConfig` export has been replaced with `loadClosestConfig` and `loadConfigFromProject` exports.
@bendemboski
Copy link
Author

@dfreeman I've addressed your feedback in a separate commit that I'll squash into the first commit if it all looks good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants