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

feat: add types for cds.build #266

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

stockbal
Copy link
Contributor

@stockbal stockbal commented Oct 3, 2024

Adds types for cds.build to have proper type support when writing a custom CDS build plugin

@daogrady
Copy link
Contributor

daogrady commented Oct 8, 2024

Hi Ludwig,

thanks for this contribution! We're actually currently looking into how we can properly offer cds-dk types, hopefully without tainting the core cds types and causing too much confusion. I see that you have pointed out that the types are only available from dk in the comments, but I'd still like to put this on hold until we have figured some things out.

Best,
Daniel

@stockbal
Copy link
Contributor Author

stockbal commented Oct 8, 2024

Hi Daniel,

sure thing. It's not that high on my requirements list. I only created this PR because of the typescript issues in cds-plugin.js of cds-typer. I wanted to create another for cds.add as well, but put it hold because for cds.add the docs explicitly mention the use of @sap/cds-dk.

When you figured out how and where to add the dk-specific typings, just give a quick shout out, I am happy to do it 😊

Regards,
Ludwig

@hakimio
Copy link
Contributor

hakimio commented Nov 19, 2024

@daogrady Have you considered just contributing the types to DefinitelyTyped and having all sap types under @types namespace? In that case it would be easier to find the needed types (@types/cds & @types/cds-dk), there would be no need for the symlinking and you would avoid issues like #251.

@daogrady
Copy link
Contributor

Hi @hakimio ,

yes, absolutely! Going for DT was considered, but finally discarded for now for two reasons:

  1. as we'd hand over control to DT, we would become contributors for our own types. At least as of now, we would like to stay full maintainers. Basically, governance over PR would be fully community driven. While this is desirable especially for open source modules where the types are contributed to DT by third parties, in our case, where we are the owners of the implementation and the types, giving up control is less attractive.
    1.2 we would have to adhere to DT's guidelines, which would probably require some extra work to meet, and can possibly become a hindrance for changes in the future.
  2. we would be bound to the release cycle of DT. That means, both versioning and release latency would be out of our control.

While we would generally love to move in with DT, these drawbacks deterred us from doing so at the current stage. We might revisit this decision in the future.

Best,
Daniel

@hakimio
Copy link
Contributor

hakimio commented Nov 19, 2024

@daogrady That's not how DefinitelyTyped works. It's a fully automated self-service kind of platform. There are thousands of projects there. You can't maintain sth like that manually with a small team.

  1. You don't hand over control to DT. When you create a type package, you, as definition owner, will be responsible for approving or disapproving PRs. Once you approve or disapprove, a DT bot will automatically merge or close the PR. See one of the PRs to get an idea how it works.
    More info about merging PRs
  2. While the releases are fully automated and done once an hour, you still control the versioning. You can set the version in package.json to what you, as a maintainer, want to. Major and minor version numbers are set by the maintainer while patch version bump is automated.
    More info about versioning

I think you should definitely reconsider this decision since it was made on a false premise.

@hakimio
Copy link
Contributor

hakimio commented Nov 19, 2024

Also, the symlink workaround doesn't work reliably with any package manger:

  • For me doesn't work at all in pnpm monorepo, other pnpm users complain that it's either flaky or doesn't work at all as well (#1 #2)
  • Doesn't work with yarn when pnp node linker is used and node modules directory is immutable
  • Even good old npm users complain that the symlink disappears all the time and builds are failing [BUG] Symlink to sap__types is deleted after each npm command #251

@daogrady
Copy link
Contributor

Hi @hakimio ,

  1. we are aware of the DT maintainer and code owner distinction, and while yes, we as owners have the right to merge PRs, there are still restrictions put up by DT.
  • the DT bot requires a "minimum two human" involvement. As I am currently the quasi-solo maintainer, meaning I create and handle most PRs, I could not approve my own changes. While this restrictions on DT's side are very reasonable and exemplary, it is just not feasible for our current team composition.
  • the DT bot seemingly requires/ asks for changes to tests in PRs. Again, very valuable metric, but would require us to set up tests in a way outside contributors could add them. Which is currently hardly the case. These are the DT guidelines I referred to above.
  • DT has a bunch of requirements that Should (capital S) be met, e.g. certain tsconfig settings
  • I don't think you are able to keep custom GH actions once your package becomes part of DT, which we currently make heavy use of (rollup). I have not found anything about it in DT's docs, but random sampling of existing packages in DT show no actions at all. And I'd be very surprised if they'd let foreign actions run in their repository.

That is all part of "handing over control". I am not debating whether these restrictions set up by DT are good (because I think they are). It is just a matter of feasibility.

  1. yes, we'd still be in charge of setting the version in the package.json. But DT strongly encourages a sync between implementation and type version. As you know, we are currently still on version 0.y.z of cds-types, while sap/cds is on 8.y.z. I am also not very happy with the perspective of being at the mercy of DT's release cycle. Be it "in a few hours", "in a day, because the DT bot has an outage", or "eventually".

Again, we are aware of the bumps our current solution causes and the convenience moving in with DT would bring. Which is why we are likely to revisit this decision at some point. But not in the foreseeable future.

Also, the symlink workaround doesn't work reliably with any package manger:

Is working with the types field in tsconfig.json not an option for you?

Best,
Daniel

@hakimio
Copy link
Contributor

hakimio commented Nov 20, 2024

the DT bot requires a "minimum two human" involvement.

Maybe @chgeo can help review the PRs

I don't think you are able to keep custom GH actions once your package becomes part of DT

Maybe you could just run them in another GH repo and use a bot to open PRs for DT repo.

But DT strongly encourages a sync between implementation and type version.

While for you, as the maintainer, it might be convenient to make breaking changes in minor version releases, I, as the user of the library, really hate when that happens (yes, I know that the lib is in "alpha" state). The change would improve DX and would be highly appreciated by all the lib users.

DT has a bunch of requirements that Should (capital S) be met

While noImplicitAny might be an issue in few cases for this library, in most cases it can be worked around by using unknown instead of any. Other requirements make sense and shouldn't be difficult to follow.

Is working with the types field in tsconfig.json not an option for you?

Yes, it is a valid workaround, but it's a custom undocumented workaround and it's not only me who is having issues with the symlink solution. Soon there will be more and more people who are not happy with it. It's just not a good solution by any means and having to come up with your own workarounds is a pretty bad DX, imho.

we are likely to revisit this decision at some point

Don't want to take more of your valuable time with this discussion, just hope this happens sooner than later.

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

Successfully merging this pull request may close these issues.

3 participants