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

Possibility of Typescript support #5097

Open
snapwich opened this issue Apr 8, 2020 · 10 comments · May be fixed by #11166
Open

Possibility of Typescript support #5097

snapwich opened this issue Apr 8, 2020 · 10 comments · May be fixed by #11166

Comments

@snapwich
Copy link
Collaborator

snapwich commented Apr 8, 2020

Description

This ticket is to track interest in Typescript types for Prebid.js. Let me know if you think types would be useful in the ways that you use Prebid.js.

My initial thought is that we could start by typing the bid adapter interface as both a complement to the bid adapter unit tests as well as being additional documentation of that interface (would most likely stay up-to-date better than jsdoc types since it's verifiable with the typescript compiler).

Later on down the line we could choose to type things such as the public API (methods on the pbjs global). I doubt we'd ever start typing internal core code as there is much less value there and it would require a lot more effort.

Some things to consider in a typescript implementation:

  • Prebid.js will probably not be rewritten in typescript as that is a huge refactor.
  • Types could be added as ambient declarations within the prebid repo as .d.ts files that live along the relevant .js modules OR we could maintain a separate types repo within definitelytyped or in the prebid organization.
  • Bid adapters could be submitted as .ts files and we could update our webpack build to accommodate building both typescript or javascript files. This would be considered a backwards breaking change since it would break webpack builds outside of the prebid repo (such as people using prebid.js as an npm dependency) that assume all modules are javascript. Since it's backwards breaking we probably wouldn't do this until a major release down the line, which could take awhile if we choose to go this route.
  • We could accept bid adapters that are already pre-compiled to .js files as bid adapter submissions and the we could choose to either allow the .ts source files to live in this repo or require you to manage your own .ts source files and only allow generated code in this repo. Since we're still only including the generated .js code in the webpack build, this is not a backwards breaking change and could be implemented right away.

A proof of concept of the last method (submitting generated javascript with typescript source alongside) has been opened as a pull-request here: #5056

If there's some other way of using types not listed here that you think could be beneficial please feel free to comment.

Other information

Previous ticket related to types: #1122

@muuki88
Copy link
Collaborator

muuki88 commented Aug 24, 2020

Thanks @snapwich for the detailed explanation. We use typescript for all our ad tags we build and provide our own set of types for prebid.js.

Prebid.js will probably not be rewritten in typescript as that is a huge refactor.

Airbnb released an interesting tool that might make this more feasible.

https://github.com/airbnb/ts-migrate

OR we could maintain a separate types repo within definitelytyped or in the prebid organization.

I would heavily favor hosting this inside the prebid repository. This ties the types much closer to the actual releases

Bid adapters could be submitted as .ts files and we could update our webpack build to accommodate building both typescript or javascript files. This would be considered a backwards breaking change since it would break webpack builds outside of the prebid repo (such as people using prebid.js as an npm dependency) that assume all modules are javascript. Since it's backwards breaking we probably wouldn't do this until a major release down the line, which could take awhile if we choose to go this route

That would be amazing. We had quite a few bugs, because of different formats things can have, e.g.

  • size definition: '300x250' or [300, 250] or [[300, 250]]`
  • bid adapter parameters had types, invalid types, no examples

I doubt we'd ever start typing internal core code as there is much less value there and it would require a lot more effort.

I wouldn't say that. There are quite a few very complex modules with very complex data structures. I'm always having a hard time reading the code as the types are never clear nor have names. This makes it much harder for external publishers / advertisers to contribute.

So overall I think adapting typescript would be beneficial for

  • external contributions
  • bug rate
  • documentation effort
  • ease of adapter implementation
  • reduce of unit testing ( half of the unit tests check that the things you put in are transformed into another thing)

@gglas
Copy link

gglas commented Feb 22, 2021

@snapwich Hey Rich -- how's this initiative going? Worth discussing next steps in our next meeting?

@snapwich
Copy link
Collaborator Author

@gglas It's not going right now, haha. I reverted my PoC and just submitted generated JS to get the adapter merged; but yes, it is worth discussing next steps I think. I still think there is value here that we could gain.

@gglas
Copy link

gglas commented May 12, 2021

@nickjacob Patrick asked me to ping you in on this -- we'll be strategizing on our rollout of typescript over the next few months, would love additional help and input!

@muuki88
Copy link
Collaborator

muuki88 commented May 12, 2021

I wonder if #5001 would also be an option that can be discussed. If migrating prebid.js to typescript is a huge effort, then maybe this time is better spent setting up a "new" prebid with prebid right from the start, but with the features defined there.

Prebid.js will be around for quite sometime, but the shift to prebid server has started and a lot adapters and modules will be irrelevant as these are already (or hopefully will be) available on prebid server.

This is a wild suggestion, but regarding limited resources this maybe a good option.

@snapwich
Copy link
Collaborator Author

@muuki88 i agree, if a re-write of prebid.js were to happen i'd propose that it should be written in typescript. however, the #5001 proposal is essentially for an adapter-less version of prebid.js where, i think, the most value for typings would reside: typing the prebid -> adapter interface. for that reason i think the discussion of introducing types to this version of prebid.js is still really important and needs to be considered.

@gglas
Copy link

gglas commented Jul 19, 2021

@snapwich would this be a breaking change? if not, we can just prioritize in the feature queue and not having to wait for a new version.

@gglas gglas added the feature label Jul 19, 2021
@mxdvl
Copy link
Contributor

mxdvl commented Jul 23, 2021

Just wanted to chime in and say that we have partial types for Prebid, based off the documentation, and that can be seen as a WIP here

I would be happy to help bring some of these as *.d.ts files to provide types. It would be especially helpful for accessing window.pbjs methods safely. Currently we still have quite a few unknowns because we only aim to make these types useful for us.

The bidResponses could also be typed by individual modules, as we’ve done for XaxisBidResponse

@patmmccann
Copy link
Collaborator

potentially fixed by #5218

@patmmccann
Copy link
Collaborator

@muuki88 muuki88 linked a pull request Mar 3, 2024 that will close this issue
12 tasks
@patmmccann patmmccann linked a pull request Mar 4, 2024 that will close this issue
12 tasks
@patmmccann patmmccann moved this from Needs Req to PR submitted in Prebid.js Tactical Issues table Sep 10, 2024
@patmmccann patmmccann moved this from PR submitted to In progress in Prebid.js Tactical Issues table Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In progress
Development

Successfully merging a pull request may close this issue.

6 participants