Precommit hooks should not require compiling Airbyte

[sherifnada]

Current Behavior

Some users have reported running into issues like the following when trying to commit or push code unrelated to the webapp:

[error] src/components/DropDown/DropDown.tsx: SyntaxError: Declaration or statement expected. (234:1)
                [error]   232 | export default DropDown;
                [error]   233 | export { DropDown };
                [error] > 234 | export type { DropdownProps };
                [error]       | ^
                [error]   235 |

Steps to Reproduce

1. Unclear exactly how, but a couple of users have mentioned this now.

Severity of the bug for you

Medium -- this is a bad user experience for someone not touching the webapp

Airbyte Version

latest: 0.17.0-alpha

[sherifnada]

@Jamakase what is required to fix this?

[jamakase]

@sherifnada not quite sure what's the problem here. I believe they may need to clear their old node_modules and run npm install. As you see on fresh CI everything is building fine.

[sherifnada]

is there any way to decouple the githooks from the code in the webapp directory or install it totally separately? if a user accidentally stops an npm build halfway through or somehow their node_modules gets messed up, but they aren't doing anything in the webapp folder, it's confusing that they may have to debug things in the webapp directory (especially if they are not a JS/TS dev)

[sherifnada]

The context is: I've heard a couple of users who are developing on connectors say that they get this error. Is it possible to check these compiled hooks in as part of the repo itself so that it is unrelated to anything in the build itself?

[jamakase]

I do not think we need it: precommit hook is run only over the changes in the commit. So if nothing was changed in webapp - then there will be no error.

We use husky ( https://github.com/typicode/husky ) lib for formatting of code before the commit, so we actually don't have what to check-in to repo as this config is already in package.json and precommit hook is created only on npm install

Formatting won't be applied, if husky lib was not installed in airbyte-webapp. So it is quite safe to assume, that if node_modules were installed - well, then this precommit hook should pass when something was changed in the webapp repo. If nothing was changed or no husky lib was installed - then precommit hook won't be fired.

I do not think that's an issue that should be addressed by FE as that's a sort of payoff we pay for mono-repo, because on gradle build we also build webapp, as a result dependencies are installed even though user may not need them.

What will help to fix this error: option 1 - rm -rf ./node_modules && npm install option 2 - rm -rf ./node_modules/husky

[sherifnada]

@Jamakase I understand that this is both not very likely to happen and also a "reasonable" consequence of having a monorepo. But ultimately it is something that: a) could (and does) happen b) leads to a bad contributor experience

And you're right: it's a simple fix. But unless someone is familiar with the structure of the monorepo or JS, they probably will:

1. spend a few minutes trying to figure it out
2. if they can't, they need to come to our Slack channel and someone has to tell them "nuke node_modules" (if they know if that is the right solution immediately)

There will be overhead/delay in doing this. This whole process could take a few hours if they are in a different timezone. That's a few hours that a contributor can't run git commit because the pre-hooks might be broken because of something unrelated to what they're working on. Of course they could nuke the repo and start over or they could google the problem, find out about git commit --no-verify, but this will probably be after a few minutes (or more) of frustration. This is not a great contributor experience.

My goal is to make the contributor experience as flawless as possible by making the chances of this happening 0%. Yes it has solutions, but it would be a better contributor experience if the problem had no chance of happening. I get it -- it's annoying because we want to have our cake (monorepo) and eat it too (make contributors not have to think about the monorepo), but I do think it leads to a better contributor experience.

this being said, I think we can find a pretty reasonable middle ground here. Is it possible to configure Husky to always skip hooks unless you have an environment variable set? (sort of like the opposite of skipping only when HUSKY_SKIP_HOOKS=1 like described here).

[jamakase]

Again - it's not possible to disable husky hook because it is not a right way to go.

You are proposing not to improve dev experience but to worsen frontend dev experience. I bet that if husky is disabled - any other guy won't be able to commit because he will need to understand where he needs to run prettier, how to format it, what variable to set and etc. So someone who will try to change anything on UI side won't be able to change it without spending significant amount of time.

The problem above appeared after a lot of library updates ( prettier, ts, forms, eslint). So it is quite an expected behavior that it is required to update dependencies.

It has occurred numerous times before when I have to run 'gradle clean build' or some additional steps to make app building locally because of backend changes and I do not see, why we should make life of UI contributors harder, just because somebody can't run it once after we upgrade bunch of libs.

What is more, I suppose we are not going to have any libs upgrade in foreseeable future, especially typescript upgrade which causes problems here.

If you have any other solutions how to make life of all contributors better - let's discuss it. But disabling husky for everybody isn't an acceptable way. It will just move the problem from one group of people to another one and even though amount of UI contributors in this repo is fairly small - it looks for me at least dishonest to not think about them.

[sherifnada]

spoke with @Jamakase offline: he believes this error happened due to a recent major typescript version upgrade and shouldn't generally be happening. This combined with the fact that we are rejiggering our contribution guidelines to not use Gradle should make the probability of this issue happening very close to 0.

If this continues to happen we'll remove husky precommit hooks and require that they are run manually as part of the dev workflow for contributing to the UI.

Closing this issue for now.