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

Upgrade ts-proto with fix for noImplicitReturns. #3909

Merged
merged 2 commits into from
Dec 9, 2021

Conversation

absoludity
Copy link
Contributor

Signed-off-by: Michael Nelson minelson@vmware.com

Description of the change

Replaces #3906 . The current version of ts-proto which we use generates typescript that fails our typescript lint checks. This means that any new PR which needs to generate, fails to pass CI.

cc @antgamdia I'm not 100%, but it looks like you may have manually updated the generated content in this commit so that it would pass? I thought of doing the same but we need to be able to work with the generated content.

I've created an issue upstream to see if it's something that may be fixed upstream, in stephenh/ts-proto#432

For now I've just disabled the noImplicitReturns check. Longer term we should re-enable this and also generate these files during our build process so that this is caught immediately when the ts-proto package was upgraded (see #3908)

Benefits

We can continue to modify protobuf files without breaking the build.

Possible drawbacks

Applicable issues

Additional information

Signed-off-by: Michael Nelson <minelson@vmware.com>
@boukeversteegh
Copy link

Hey @absoludity , thanks for opening an issue in ts-proto, we will fix the issue there.

In the meantime, I thought the following might be helpful. When generated code doesn't match your project's linting rules, you could also disable linting on the generated code, or exclude a violated rule for a particular folder or file. That way you won't be blocked and don't need to disable the rule for your whole project. It's also handy for cases where the rules of the two projects contradict each other, and it cannot be fixed upstream.

For example, you could do this:

// .eslintrc.js
module.exports = {
  // exclude entire directory from linting
  ignorePatterns: ['src/lib/**'],
  overrides: [
    // disable specific rule in directory
    {
      files: ['somedirectory/**/*.ts'],
      rules: {
        '@typescript-eslint/no-undef': 'off',
      },
    },
  ],
}

I'm not an expert on eslint, but this configuration worked for me.

@boukeversteegh
Copy link

PR ready stephenh/ts-proto#436

@absoludity
Copy link
Contributor Author

absoludity commented Dec 8, 2021

Excellent, thanks @boukeversteegh ... now I can just update ts-proto instead :) And thanks for the info about the eslintrc..., I did briefly look for a way to suppress it for a particular file in the tsconfig, but missed the overrides in .eslintrc.

Signed-off-by: Michael Nelson <minelson@vmware.com>
@absoludity absoludity changed the title Disable noImplicitReturns for now. Upgrade ts-proto with fix for noImplicitReturns. Dec 8, 2021
@absoludity
Copy link
Contributor Author

Rubber-stamping this one to unblock changes to proto files.

@absoludity absoludity merged commit 1ffb388 into master Dec 9, 2021
@absoludity absoludity deleted the fix-typescript-proto-gen branch December 9, 2021 01:03
@castelblanque
Copy link
Collaborator

@absoludity After this change (containing the upgrade of ts-proto): could we get rid of the generated TS source code for Dashboard?

@absoludity
Copy link
Contributor Author

@absoludity After this change (containing the upgrade of ts-proto): could we get rid of the generated TS source code for Dashboard?

It's unrelated to this change/upgrade. We may want to do so but that'll be part of the discussion at #3908 .

@antgamdia
Copy link
Contributor

Sorry, I've been OOO for a while.

cc antgamdia I'm not 100%, but it looks like you may have manually updated the generated content in this commit so that it would pass? I thought of doing the same but we need to be able to work with the generated content.

Yep, you're totally right. I added a return undefined to pass the linter issue (albeit being consistent with the method's signature) after the ts-proto dep upgrade while developing the service account thing. I forgot to look into the issue in-depth and I totally miss that change when committing and sending the PR. My bad. I'm sorry :(

Thanks for catching the issue, Michael, and special thanks to the ts-proto team; what a quick solution!!

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.

4 participants