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

isLegacyProjectConfig has logical error. #1416

Open
4 tasks
charleslai2000 opened this issue Oct 13, 2023 · 3 comments
Open
4 tasks

isLegacyProjectConfig has logical error. #1416

charleslai2000 opened this issue Oct 13, 2023 · 3 comments

Comments

@charleslai2000
Copy link

Issue workflow progress

Progress of the issue based on the Contributor Workflow

  • 1. The issue provides a reproduction available on GitHub, Stackblitz or CodeSandbox

    Make sure to fork this template and run pnpm run generate in the terminal.

    Please make sure the Codegen and plugins version under package.json matches yours.

  • 2. A failing test has been provided
  • 3. A local solution has been provided
  • 4. A pull request is pending review

The logic to decide whether a config is legacy has error.

To Reproduce
Steps to reproduce the behavior:

Correctly get new configuration.

Environment:

  • OS: Ubuntu 22.04
  • GraphQL Config Version: 5.0.2
  • NodeJS: 20
export class GraphQLProjectConfig {
    constructor({ filepath, name, config, extensionsRegistry, }) {
        this.filepath = filepath;
        this.dirpath = dirname(filepath);
        this.name = name;
        this.extensions = config.extensions || {};
        if (isLegacyProjectConfig(config)) {
            this.schema = config.schemaPath;
            this.include = config.includes;
            this.exclude = config.excludes;
            this.isLegacy = true;
        }
        else {
            this.schema = config.schema;
            this.documents = config.documents;
            this.include = config.include;
            this.exclude = config.exclude;
            this.isLegacy = false;
        }
        this._extensionsRegistry = extensionsRegistry;
    }

In this code, when project configuration has "include" or "exclude" keywords, is LegacyProjectConfig will return true. This is weird.

@dimaMachina
Copy link
Collaborator

Where did you see it?

export function isLegacyProjectConfig(config: IGraphQLConfig): config is IGraphQLProjectLegacy {
return (
(config as IGraphQLProjectLegacy).schemaPath !== undefined ||
(config as IGraphQLProjectLegacy).includes !== undefined ||
(config as IGraphQLProjectLegacy).excludes !== undefined
);

@charleslai2000
Copy link
Author

charleslai2000 commented Oct 14, 2023

The code piece is in "src/project-config.ts". The logic flaw is when some config file has provided includes or exludes ( in my case, @graphql-eslint will refuse lint file if don't support includes), isLegacyProjectConfig will treat it as legacy. But includes and excludes also are read when not in legacy mode.

So, if you provide a config with schema, includes field, it will always to treat as legacy mode, and schema filed will be undefined. I spent 1d to find this problem when config my graphql LSP and eslint plugin.

I don't kown why this condition is needed? Can you judge it as simple as " if (config.schemaPath !== undefined) "?

@dimaMachina
Copy link
Collaborator

So the problem is not with graphql-config but with graphql-eslint?

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

No branches or pull requests

2 participants