Skip to content

feat(cli): add message when all files compile successfully in nango dev #3908

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

Merged
merged 19 commits into from
Apr 24, 2025

Conversation

kaposke
Copy link
Contributor

@kaposke kaposke commented Apr 15, 2025

Description

We currently have no feedback for when all files compile successfully, leading to a confusing DX when the last thing you see is the last error you got.

This PR adds this:
image

Feel free to suggest another message

To achieve this, I had to keep track of failed files and tweak how things were done. I believe I've fixed a couple bugs as a tangent. I'll add comments to the relevant sections with more details.

Testing

Please checkout to the branch and run nango dev in a nango project. Break and fix typescript files and the config file, in mixed order, to see if the DX is looking good. The No compilation errors. message should only show up when everything is looking good, both with TS files as well as nango.yaml.

@kaposke kaposke requested a review from a team April 15, 2025 14:11
Copy link

linear bot commented Apr 15, 2025

}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure you need a function for that. Inlining the if would be good enough imho

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If that's a NIT, I prefer having it. I generally like to avoid duplicating user-facing messages. It should be updatable in a single place when needed.

Copy link
Collaborator

@bodinsamuel bodinsamuel left a comment

Choose a reason for hiding this comment

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

Seems to work, just one comment

  • When I use dev locally it parse the nango.yaml once per file and every time I save a script too, which is super noisy when you have warning (and also probably not necessary)

Unrelated

  • compile.service.ts:145 it's missing chalk.red, if you can take the opportunity to fix it that would be nice 🙏🏻

@@ -43,6 +43,7 @@
"test:unit": "vitest",
"test:integration": "vitest --config ./vite.integration.config.ts",
"test:cli": "vitest --config ./vite.cli.config.ts",
"test:cli:update-snapshots": "vitest --config ./vite.cli.config.ts --update",
Copy link
Collaborator

Choose a reason for hiding this comment

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

note you can just press u when running tests to update snapshots

@kaposke kaposke requested a review from bodinsamuel April 16, 2025 16:42
Copy link
Collaborator

@bodinsamuel bodinsamuel left a comment

Choose a reason for hiding this comment

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

I still have one nango.yaml parsing per file or per save.
and no success message at the end

Screen.Recording.2025-04-17.at.10.55.42.mov

@kaposke
Copy link
Contributor Author

kaposke commented Apr 17, 2025

I still have one nango.yaml parsing per file or per save. and no success message at the end

Screen.Recording.2025-04-17.at.10.55.42.mov

Oh thanks for the video. I'll try to reproduce it.

Copy link

gitguardian bot commented Apr 22, 2025

⚠️ GitGuardian has uncovered 2 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request
GitGuardian id GitGuardian status Secret Commit Filename
16627964 Triggered Username Password 042f8b5 packages/shared/lib/utils/utils.unit.test.ts View secret
16627965 Triggered Generic Password 042f8b5 packages/shared/lib/utils/utils.unit.test.ts View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secrets safely. Learn here the best practices.
  3. Revoke and rotate these secrets.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

@bodinsamuel
Copy link
Collaborator

You get no message when you have an external lib, put some helpers outside your sync folder, and import them in your sync. It's a common pattern so it should be handled

@kaposke
Copy link
Contributor Author

kaposke commented Apr 23, 2025

You get no message when you have an external lib, put some helpers outside your sync folder, and import them in your sync. It's a common pattern so it should be handled

Thanks. I was able to reproduce it and fix it. The problem was that non sync/action files are not compiled on change. They are only compiled when the sync/action is changed, and they get bundled together in the output JS. That actually raises another problem, which is beyond the scope of this PR. Raised NAN-3152

Anyways, ignored files were being treated as failures. By changing that the message now works. However, if you break the dependency file, it will only show compilation errors when you change it's dependent file.

@bodinsamuel
Copy link
Collaborator

Makes sense, thanks for investigating. I think the compilation is unnecessarily complex right now. We had plan in (not moving much unfortunately) to rewrite how we handle scripts and compilation that would remove the need for all of this

@kaposke kaposke merged commit 180cf75 into master Apr 24, 2025
16 checks passed
@kaposke kaposke deleted the gui/NAN-2985/feat-successful-compilation-message branch April 24, 2025 16:37
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.

3 participants