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

refactor(npm): Use schema for PackageSource parsing #22314

Merged
merged 3 commits into from May 19, 2023

Conversation

zharinov
Copy link
Collaborator

Changes

Context

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

@zharinov zharinov requested review from viceice and rarkins May 19, 2023 13:28
Copy link
Collaborator

@rarkins rarkins left a comment

Choose a reason for hiding this comment

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

What's the advantage of this approach? It's longer code, and less readable to a non-zod person. So is there a "bigger picture" advantage?

@zharinov
Copy link
Collaborator Author

What's the advantage of this approach? It's longer code, and less readable to a non-zod person. So is there a "bigger picture" advantage?

It helps to decouple data validation (declarative part) and data transformation. They're separated, but often live together since usually we want them to be performed one right after another. Refactored code inside transform(...) is simpler than same code before refactoring, I even would say it's dumb, e.g.:

...

if (homepage) {
  result.homepage = homepage;
}

if (sourceUrl) {
  result.sourceUrl = sourceUrl;
}

...

One reason why type schema and its data transformation are better to live together: the only place when user can use the outer-world type (e.g., from npm or composer endpoints) is the .transform(...) functional parameter. Once transformed, this data is simpler to use, e.g. less defensive checks, etc. In many cases transforms can just return ReleaseResult that is ready to be immediately returned as the getPackageRelease() result. Otherwise, user still left with way more structured data compared to its raw form.

One more reason for this approach is the granularity and composability: we don't want to just validate the whole nested data structure against the strict schema. We want to do the best for accepting the incomplete data as the valid structure work we can work with: default values, catch clauses and logging the specific information. We don't want to reject all the data just because some not important field is invalid. Once done with particular fields, we then combine them together into the single structure for further usage.

The downside is the syntax that is hard to parse without prior knowledge of the library. But code I'm refactoring very often also is not easy to read. Seems like we're choosing between two types of hard to read code, but with new approach providing stronger type safety guarantees, while for another one we often assume everything will be okay and types we're describing are matching the data we receive from the outside world.

@zharinov
Copy link
Collaborator Author

BTW I think it would be way easier to read if the union was renamed to alt

@rarkins rarkins added this pull request to the merge queue May 19, 2023
Merged via the queue into renovatebot:main with commit 3164401 May 19, 2023
11 checks passed
@rarkins rarkins deleted the refactor/npm-package-source-schema branch May 19, 2023 17:48
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 35.96.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants