DEV Community: Jiri Spac

The Right Time to Approve a Pull Request: Embracing Improvement Over Perfection

Jiri Spac — Fri, 09 Feb 2024 14:54:52 +0000

In the ever-accelerating world of software development, the Pull Request (PR) review process stands as a crucial checkpoint. Especially in the time of AI. It's the confluence where innovation meets critique, quality is meticulously refined, and rubber meets the road. Yet, a persistent specter haunts many development teams: the unspoken belief that a PR must achieve an almost mythical state of perfection before earning the coveted "Approved" stamp. This well-intentioned pursuit of flawlessness, however, often clashes fundamentally with the iterative, adaptive spirit of Agile development.

Embracing Momentum: Improvement as the True North

Agile methodologies are built on a foundation of continuous improvement—a relentless cycle of building, measuring, and learning through small, manageable increments. This powerful philosophy should permeate every aspect of our work, especially how we evaluate PRs. A PR's primary mission shouldn't be to present a flawless masterpiece, but rather to represent a tangible step forward. If a proposed change demonstrably enhances the User Experience (UX) or smooths out friction in the Developer Experience (DX), it carries inherent value and generally warrants approval. Consider these dimensions:

Elevating the User Experience (UX):
Improvements here can span the spectrum – from subtle interface adjustments that clarify user journeys, to significant feature enhancements that unlock new levels of satisfaction. Each positive modification builds towards a superior product. Holding back these valuable increments in pursuit of an unattainable "perfect" state risks stagnation. We delay tangible benefits for users, potentially leaving them grappling with known pain points longer than necessary, which can erode satisfaction and trust.
Refining the Developer Experience (DX):
Often underestimated, the DX is paramount to a team's velocity and well-being. A PR that untangles complex logic, boosts code clarity, pays down technical debt, or introduces helpful tooling makes the entire development lifecycle more efficient and sustainable. While individual DX improvements might seem minor, their compound effect is profound. Approving such PRs keeps the development engine running smoothly, fosters motivation, combats burnout, and ultimately enables the team to deliver value faster.

The Agile ethos thrives on adaptation, feedback loops, and incremental progress. Applying this lens to PR reviews cultivates a culture where collaboration flourishes, learning is continuous, and adaptability is paramount. The goal isn't necessarily immaculate code in every single PR, but rather collective forward motion, refining our creation with each contribution.

The Perils of Delay: When Good PRs Languish

Waiting for perfection can have tangible negative consequences. Imagine these scenarios:

Scenario 1: The Lingering UX Annoyance: A developer submits a PR with a simple fix: clarifying the label on a confusing button that generates frequent user support tickets. The core logic is sound, but the reviewer blocks it, demanding minor stylistic changes or unrelated refactoring. Weeks pass. While the team debates stylistic nuances, users continue to stumble, support remains burdened, and a straightforward improvement sits gathering digital dust.
Scenario 2: The DX Bottleneck: A thoughtful PR refactors a notoriously complex and brittle module, making it significantly easier to understand and modify. It passes all tests and achieves its goal. However, it's held up because a reviewer wants a different (but functionally equivalent) naming convention applied throughout. Meanwhile, two other developers need to build features touching that same complex module. They are forced to grapple with the old, difficult code, slowing their progress and increasing the risk of introducing new bugs – all because a beneficial refactoring was unnecessarily delayed.

These examples highlight how demanding perfection over progress can actively hinder the team and negatively impact both users and developers.

Drawing the Line: When Approval is Not the Answer

While we champion progress, quality cannot be sacrificed wholesale. There are clear situations where rejecting a PR is not just appropriate, but essential for maintaining the integrity and stability of the product. The key is to approach rejection constructively, transforming it into a learning opportunity rather than a point of discouragement. Here are non-negotiable reasons for rejection:

Continuous Integration (CI) Failures: If the automated CI pipeline flags errors (and these aren't attributable to flaky tests), it's a critical stop sign. A red build signals that the changes have introduced bugs, broken existing functionality, or failed to adhere to established project standards. Ensuring a green CI build is a fundamental prerequisite for merging code, safeguarding the application's baseline stability.
Breaking the Primary Use Case: Any PR that compromises the core, essential functionality of the application must be rejected. These primary use cases represent the fundamental value delivered to users. Disrupting them, even unintentionally, can have severe repercussions. Rigorous verification that the main features remain fully operational is mandatory before approval.
Introducing Critical Performance Regressions: Performance is often a key feature in itself. A PR that causes a measurable, significant slowdown in critical operations harms the user experience and can undermine the application's viability. Such regressions should be caught via performance benchmarks or profiling tools integrated into the development lifecycle. If a PR demonstrably degrades performance beyond acceptable thresholds, it needs revision.

When rejection is necessary, clarity is paramount. Explain precisely what needs to be addressed. Utilizing standardized formats like Conventional Comments can significantly aid in providing actionable, unambiguous feedback.

The Unsung Hero: A Robust Testing Strategy

Underpinning this entire philosophy is the non-negotiable requirement of a comprehensive testing strategy. Unit tests, integration tests, end-to-end (E2E) tests, and potentially performance tests form the safety net that allows us to embrace incremental improvement with confidence. This robust test suite acts as an automated guardian, catching regressions and ensuring that approved changes don't inadvertently destabilize the application. Without solid testing, the "improvement over perfection" approach becomes significantly riskier.

Rejection as a Catalyst for Growth

A rejected PR shouldn't signify failure, but rather a need for course correction. It's an invaluable feedback loop. When rejecting, deliver clear, constructive criticism outlining the specific issues and suggesting pathways to resolution. Encourage the contributor to iterate and resubmit. Frame the review process as a collaborative partnership aimed at achieving shared quality goals, not as a gatekeeping exercise designed to block contributions.

Gauging Team Health: The Rejection Rate Metric

Does this mean approving virtually every PR? Ideally, in a perfectly aligned team, yes. But reality dictates rejections will happen. The rate of rejection, however, can be a fascinating indicator of team dynamics and process health.

High Rejection Rate (e.g., >40%): This often signals deeper issues. Is the team misaligned on quality expectations? Are requirements poorly defined? Is there a lack of mentorship for junior developers? Or, critically, are essential automated checks missing, allowing fundamentally broken PRs to even reach the review stage?
Very Low Rejection Rate (e.g., <5-6%): While seemingly positive, this could indicate rubber-stamping. Are reviewers engaging critically, or just clicking "Approve" to clear their queue? Thorough reviews are still vital, even with highly skilled teams.
Healthy Range (e.g., 6-14%): Teams operating in this zone often demonstrate good alignment, strong testing practices, and effective collaboration. Rejections occur for valid reasons, and feedback loops are generally constructive.

Remember, even within elite teams where code quality is consistently high, the PR process serves another vital function: knowledge dissemination. It combats information silos and ensures multiple team members gain familiarity with different parts of the codebase.

Finding the Balance

The decision to approve or reject a Pull Request hinges on a pragmatic balance. Prioritize tangible improvements to the product or the development process. Set clear, high-level bars for rejection based on critical failures, backed by a strong testing infrastructure. Foster a culture where feedback is constructive, collaboration is valued, and progress is consistently celebrated. Every thoughtfully approved PR represents forward momentum on your product's journey. Reject judiciously, approve progressively, and build better software, together.

How to let lambda access the internet outside of your VPC

Jiri Spac — Sat, 14 Jan 2023 11:10:25 +0000

This article should not exist. It's absolutely a huge problem when something as trivial, as mundane, as obvious generates such a confusion between your users.
When I was first greeted with this:



Error: There are no 'Private' subnet groups in this VPC. Available types: Public
    at LookedUpVpc.selectSubnetObjectsByType (/home/capaj/work-repos/top-secret/node_modules/aws-cdk-lib/aws-ec2/lib/vpc.js:1:4998)
    at LookedUpVpc.selectSubnetObjects (/home/capaj/work-repos/top-secret/node_modules/aws-cdk-lib/aws-ec2/lib/vpc.js:1:3724)
    at LookedUpVpc.selectSubnets (/home/capaj/work-repos/top-secret/node_modules/aws-cdk-lib/aws-ec2/lib/vpc.js:1:1454)
    at Function.configureVpc (/home/capaj/work-repos/top-secret/node_modules/aws-cdk-lib/aws-lambda/lib/function.js:1:18626)
    at new Function (/home/capaj/work-repos/top-secret/node_modules/aws-cdk-lib/aws-lambda/lib/function.js:1:5676)
    at /home/capaj/work-repos/top-secret/cdk/LambdaStack.ts:93:30
    at Array.forEach (<anonymous>)
    at new LambdaStack (/home/capaj/work-repos/top-secret/cdk/LambdaStack.ts:88:17)
    at Object.<anonymous> (/home/capaj/work-repos/top-secret/bin/backend.ts:58:3)
    at Module._compile (node:internal/modules/cjs/loader:1159:14)

I thought it's going to be easy. I'll just flip a switch somewhere and we will be done.
Nope. AWS Cloud had much more in store for me that day. Hell for a few more days.

These were stack overflow answers I was desperately trying to understand:

https://stackoverflow.com/questions/52992085/why-cant-an-aws-lambda-function-inside-a-public-subnet-in-a-vpc-connect-to-the
https://stackoverflow.com/a/55267891/671457
https://stackoverflow.com/a/43669759/671457
https://stackoverflow.com/a/50280780/671457 (this one is quite confusing with the ENI)

This AWS CDK error shows up, when you try to deploy lambdas into your default VPC. If you created your VPC without a "Private" subnet like me and you have a TS infa code like this:



      const lambdaFunction = new lambda.Function(this, `${cron.name}-cron`, {
        memorySize: cron.memorySize ?? 2048,
        timeout: cdk.Duration.minutes(15),
        runtime: lambda.Runtime.NODEJS_18_X,
        architecture: Architecture.ARM_64,
        handler: `${cron.name}.${cron.name}`,
        code: lambda.Code.fromAsset(path.join(__dirname, '../dist'), {
          exclude: [
            ...otherLambdas.map((cron) => `${cron.name}.js`),
            ...otherLambdas.map((cron) => `${cron.name}.js.map`)
          ]
        }),
        environment: envVars,
        securityGroups: [lambdaSecurityGroup],
        vpc,
        vpcSubnets: { subnetType: SubnetType.PRIVATE_WITH_EGRESS },
        role: lambdaVPCExecutionRole
      })

It will be failing. At first I though I can just switch a subnet into Private by going into AWS console https://eu-west-1.console.aws.amazon.com/vpc/home?region=eu-west-1#subnets:
but it's not as simple.
This is what you need to do:

create a new subnet. You don't choose whether it is public or private in this step.
create a new network ACL with everything blocked- AWS will recognize the subnet as private if it sees network ACL blocking remote access
now click
go into you cdk.context.json and delete your VPC there. This will force AWS CDK to refresh the subnets in your VPC.

The last remaining bit for the subnet to be PRIVATE_WITH_EGRESS is to create a NAT and add it to your routing table.

name it, ⚠️ select a PUBLIC subnet. This is where a lot of people make a mistake. If you choose a private subnet when creating NAT, it will not work. NAT needs to be in a public subnet with internet gateway(IGW is available by default).
go create a new route table
add your nat as target for 0.0.0.0/0 route
associate the new route table with your subnet:
go into you network ACL add the local ip address of the NAT gateway to both Inbound and Outbound rules. This is needed because ACLs are stateless, whereas with security groups it is enough to have just one sided rule. For ACL ALWAYS make sure to have rules for both sides.
cdk deploy

🚀 Congratulations, now you have deployed your lambda into a PRIVATE_WITH_EGRESS subnet and they will be able to do network call outside of their VPC.

Lambdas are a powerful tool, but this should ideally be a single switch somewhere. A senior developer with no prior AWS experience shouldn't need to spend more than 15 minutes to do this. Some reportedly spent several hours, me included.
ATM it's a very error prone process with a lot of steps to achieve. Thankfully for going the other way, there are function URLs, which greatly simplify deployment and provisioning of lambdas when you need to call them from outside of the VPC. If anyone from AWS team reads this, please consider doing the similar simplification for the other way too.

How to send a file in form of a Buffer as a FormData in node.js

Jiri Spac — Tue, 10 Jan 2023 20:30:22 +0000

This is slightly modified excerpt from https://github.com/octet-stream/form-data#readme

import {Readable} from "stream"

import {FormData} from "formdata-node"

class BlobFromStream {
  #stream: Readable
  size: number

  constructor(stream: Readable, size: number) {
    this.#stream = stream
    this.size = size
  }


  stream() {
    return this.#stream
  }

  get [Symbol.toStringTag]() {
    return "Blob"
  }
}

const content = Buffer.from("Stream content")

const stream = new Readable({
  read() {
    this.push(content)
    this.push(null)
  }
})

const form = new FormData()

form.set("stream", new BlobFromStream(stream, content.length), "file.txt")

await fetch("https://httpbin.org/post", {method: "post", body: form})

just in case anyone would be struggling with this same as me.

sending a file with curl POST needs a magic at sign

Jiri Spac — Sat, 31 Dec 2022 00:37:58 +0000

curl -X POST -H "Content-Type: multipart/form-data" -F "file=myAwesomeFile.pdf"

does not work. The magic part os prepending at sign in the beginning of file name.

curl -X POST -H "Content-Type: multipart/form-data" -F "[email protected]"

Just in case someone struggles with this same as me.

Overhead of using REST instead of Graphql with node.js

Jiri Spac — Fri, 04 Feb 2022 18:51:05 +0000

There are quite a few misconceptions when it comes to graphql. It cannot be cached on http protocol level, it has to return 200 code even when there are errors, it kills fluffy kitten every time you make a mutation.
In this short post I would like to address one of them in particular. No not the one about the kittens.

// Detect dark theme var iframe = document.getElementById('tweet-1410228246487126017-4'); if (document.body.className.includes('dark-theme')) { iframe.src = "https://platform.twitter.com/embed/Tweet.html?id=1410228246487126017&theme=dark" }
// Detect dark theme var iframe = document.getElementById('tweet-1433825642227781632-945'); if (document.body.className.includes('dark-theme')) { iframe.src = "https://platform.twitter.com/embed/Tweet.html?id=1433825642227781632&theme=dark" }
// Detect dark theme var iframe = document.getElementById('tweet-1307017642478247937-908'); if (document.body.className.includes('dark-theme')) { iframe.src = "https://platform.twitter.com/embed/Tweet.html?id=1307017642478247937&theme=dark" }
// Detect dark theme var iframe = document.getElementById('tweet-1278941977413902336-279'); if (document.body.className.includes('dark-theme')) { iframe.src = "https://platform.twitter.com/embed/Tweet.html?id=1278941977413902336&theme=dark" }

Let's analyse and dissect what these developers might be talking about.

Overhead on the Client side

As a consumer of graphql APIs you need to send an exact list of fields you want back. If your model is complex and it has lots of fields, this does indeed introduce extra DX complexity writing the API request.
This "overhead" only exists without tooling.
Every graphql API should come with graphiql/playground bundled. These tools complete the query for you, so there is little to no developer time lost. On the contrary developers can explore the API at the same time as they query.

Even when it doesn't come bundled with the API, developer can run a variety of graphql clients on their end.
For example graphiql does it here.
Hence I came to a conclusion that it's not this overhead that these people are talking about.This is too trivial and solvable with tooling everyone uses by default. This cannot be it.

Overhead on the Server side

This one is harder to crack. Is this actually a myth? There are myriad ways to write a backend in both of these. In order to prove I knew I had to get my hands dirty.

I was always a fan of DRY, not DRY nazi, just a fan. So I've been actually always searching for less verbose ways to write APIs, ever since I started years ago.

REST

In the last few years I've gravitated toward stacks centered around fastify. So let that be the http server base for our comparison.
I don't think there are more succint ways to write a documented REST api other than fastify and fastify-swagger. The way fastify-swagger leverages validation schema to produce the docs is quite awesome. It reminds me of a a poor man's thing which starts with a capital "G" and it's killing kittens.

Graphql

Similarly, for the Graphql camp, I don't know anything less verbose than Typegraphql and mercurius.
Mercurius-fastify plugin has the best defaults of any GQL server and performance on whole different level compared to apollo-server.

So I've taken a bit of time today and written a single CRUD endpoint for a very simple blog. There is no authentication, no rate limiting, no caching, nothing extra-just a pure bussines logic for CRUD over one DB table.
What I do implement in both is validation and documentation as we need to have feature parity for a fair comparison.
For ORM I chose prisma, as it offers awesome generators to spare my lazy ass from writing API schemas manually.
For the DB we just use sqlite.

What was the result?
🥁
See this repo: https://github.com/capaj/node-rest-vs-graphql

These endpoints are implemented:

query for blogpost with filtering by from and to dates
query to fetch a single blogpost
mutation to create a new post, update existing one and delete a blogpost

Just for this very simple API, REST adds 21 extra lines.

I would welcome any PR regarding simplification of the REST api, because I may have missed something. Maybe there is some magical way how to have validation and documentation without all that work. Maybe I just haven't learned it yet. I am just a junior dev 4life.

I really wish there was an easier way to properly do REST apis in node.js. Personally I'll be sticking to typegraphql and mercurius.
Keep in mind that for a more complex api-where you have deeply nested relations of entities, REST becomes even more unwieldy. Graphql is natural for representing data with lots of relations allowing the consumer to nest their queries deep. In leaf resolvers you always have access to a parent when resolving a child-you can often use this to simplify your BE code.
These practices of structuring endpoints exist in REST as well, but it's all manual. You actually need to write bunch of middleware to get it to work.

One last reason for me to avoid REST is that I am quite bad at remembering to do something when I am not forced to do so by an automated system. If I was writing REST apis, I would most likely need a colleague to remind me to add that optional validation in every odd code review. This is potentially solvable in fastify type definitions. I think it's not likely it will ever get implemented. Fastify is primarily a javascript framework and the type defs are manually added just for consumers.

Final note: please if you see anyone on twitter talk about how Graphql has implementation overhead compared to REST, do send them this link: https://github.com/capaj/node-rest-vs-graphql/pull/1