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

[doc] contributing guide #3

Closed
thomasleveil opened this issue Aug 13, 2019 · 10 comments
Closed

[doc] contributing guide #3

thomasleveil opened this issue Aug 13, 2019 · 10 comments

Comments

@thomasleveil
Copy link
Contributor

thomasleveil commented Aug 13, 2019

The current development setup guide is quite light:

## Development Setup

1. Clone the repository
2. Go into repository folder
3. Run: `npm install`
4. Run: `npx lerna bootstrap --hoist`
5. Run: `npm run build` or `npx lerna exec npm run build` (if lerna is not installed)

At least, for me, it wasn't sufficient in order to get me started 😢

Maybe add:

### Requirements

     npm install -g lerna

and then make sure the code in the HEAD of master does compile with lerna run build.

Cloning 131823a, I've got the following issues:

  • error TS5014: Failed to parse file 'tsconfig.json': Unexpected token ] in JSON at position 85. which seems to be caused by trailing commas in arrays

Fixing those commas, the build then fails with:

$ lerna run build

info cli using local version of lerna
lerna notice cli v3.16.4
lerna info versioning independent
lerna info Executing command in 6 packages: "npm run build"
lerna ERR! npm run build exited 1 in 'n8n-node-dev'
lerna ERR! npm run build stdout:

> n8n-node-dev@0.2.0 build C:\Users\tleveil\workspace\contrib\n8n\packages\node-dev
> tsc

error TS5023: Unknown compiler option 'lib'.
error TS5023: Unknown compiler option 'types'.
error TS5023: Unknown compiler option 'esModuleInterop'.
error TS5023: Unknown compiler option 'strictNullChecks'.
error TS5023: Unknown compiler option 'strict'.
error TS6047: Argument for '--target' option must be 'ES3', 'ES5', or 'ES6'.

lerna ERR! npm run build stderr:
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! n8n-node-dev@0.2.0 build: `tsc`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the n8n-node-dev@0.2.0 build script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     C:\Users\tleveil\AppData\Roaming\npm-cache\_logs\2019-08-13T12_39_58_080Z-debug.log

lerna ERR! npm run build exited 1 in 'n8n-node-dev'
lerna WARN complete Waiting for 1 child process to exit. CTRL-C to exit immediately.

And there you loose potential contributors 😐

@janober
Copy link
Member

janober commented Aug 13, 2019

Hello!

Thanks a lot for creating an issue about that! And you are 100% right. It is currently more than light. Actually is the documentation one of the main reasons I did not launch n8n anywhere officially yet. Hope to imporove on that a lot till the end of this month when I want to launch it on some networks.

Will look tonight into your issue and get then back to you!

@janober
Copy link
Member

janober commented Aug 13, 2019

Hello!

I checked now. There was a problem with the build because inquirer. Somehow was not added as dependency at all (right now no idea when it got lost and why the docker builds still worked fine). Anyway that did not even cause the problem, it was actually that they apparently renamed a type and therefor it did not build anymore. After I fixed that issues and followed the instructions everything worked fine for me. I am however on Ubuntu and you seem to work on Windows, so maybe something goes wrong there.

Btw. lerna does not has to be installed for the instructions to work. I extra used npx instead of npm in step 3 so that even if lerna is not installed it will simply get it. Also mention that again in step 5 where I wrote two different commands. There the user can choose one depending on if he has lerna installed or not.
No matter what, should probably simplify that.

Will now check in Windows and then get back to you.

@janober
Copy link
Member

janober commented Aug 13, 2019

"Sadly" no luck. It also worked without a problem on Windows for me. I however now "fixed" the tsconfig.json files. Even though it did not cause any issues for me, it is apparently an issue for other people like you. So is a now brainer...

However, the error message you get and the fact that for you the tsconfig.json files made problems, even though it does not for me, makes me believe that the problem is TypeScript related. Do you maybe have an old global version installed which messes the n8n build up?

@thomasleveil
Copy link
Contributor Author

thomasleveil commented Aug 13, 2019

I'm at home now, on Linux Mint, did:

  1. clone from 86e1c4a
  2. npm install
  3. npx lerna bootstrap --hoist
  4. npx lerna exec npm run build

and the build went fine.


However, I can reproduce my previous issue if I do:

git clone https://github.com/n8n-io/n8n.git && cd n8n
git checkout 131823a14d061c7258d2a6d1c23ed62eef0e45cd
npm i
npx lerna bootstrap --hoist
npx lerna exec npm run build
Error:

 ~/workspace/contributing/n8n  ➦ 131823a  npx lerna exec npm run build
lerna notice cli v3.16.4
lerna info versioning independent
lerna info Executing command in 6 packages: "npm run build"

> n8n-node-dev@0.2.0 build /home/thomas/workspace/contributing/n8n/packages/node-dev
> tsc


> n8n-workflow@0.9.0 build /home/thomas/workspace/contributing/n8n/packages/workflow
> tsc

commands/new.ts:26:34 - error TS2694: Namespace 'inquirer' has no exported member 'Questions'.

26  			const typeQuestion: inquirer.Questions = {
    			                             ~~~~~~~~~

commands/new.ts:45:9 - error TS2571: Object is of type 'unknown'.

45  			if (typeAnswers.type === 'Node') {
    			    ~~~~~~~~~~~

commands/new.ts:50:39 - error TS2694: Namespace 'inquirer' has no exported member 'Questions'.

50  				const nodeTypeQuestion: inquirer.Questions = {
    				                                 ~~~~~~~~~

commands/new.ts:67:10 - error TS2571: Object is of type 'unknown'.

67  				if (nodeTypeAnswers.nodeType === 'Trigger') {
    				    ~~~~~~~~~~~~~~~

commands/new.ts:70:17 - error TS2571: Object is of type 'unknown'.

70  				} else if (nodeTypeAnswers.nodeType === 'Webhook') {
    				           ~~~~~~~~~~~~~~~

commands/new.ts:102:85 - error TS2694: Namespace 'inquirer' has no exported member 'Questions'.

102  			const additionalAnswers = await inquirer.prompt(additionalQuestions as inquirer.Questions);
     			                                                                                ~~~~~~~~~

commands/new.ts:104:22 - error TS2571: Object is of type 'unknown'.

104  			const nodeName = additionalAnswers.name;
     			                 ~~~~~~~~~~~~~~~~~

commands/new.ts:108:91 - error TS2571: Object is of type 'unknown'.

108  			const destinationFilePath = join(process.cwd(), `${changeCase.pascalCase(nodeName)}.${typeAnswers.type.toLowerCase()}.ts`);
     			                                                                                      ~~~~~~~~~~~

commands/new.ts:118:40 - error TS2694: Namespace 'inquirer' has no exported member 'Questions'.

118  				const overwriteQuestion: inquirer.Questions = [
     				                                  ~~~~~~~~~

commands/new.ts:129:10 - error TS2571: Object is of type 'unknown'.

129  				if (overwriteAnswers.overwrite === false) {
     				    ~~~~~~~~~~~~~~~~

commands/new.ts:143:30 - error TS2571: Object is of type 'unknown'.

143  				NodeDescriptionReplace: additionalAnswers.description,
     				                        ~~~~~~~~~~~~~~~~~


Found 11 errors.

npm ERR! code ELIFECYCLE
npm ERR! errno 2
npm ERR! n8n-node-dev@0.2.0 build: `tsc`
npm ERR! Exit status 2
npm ERR! 
npm ERR! Failed at the n8n-node-dev@0.2.0 build script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/thomas/.npm/_logs/2019-08-13T18_03_41_774Z-debug.log
lerna ERR! npm run build exited 2 in 'n8n-node-dev'
lerna ERR! npm run build exited 2 in 'n8n-node-dev'
lerna WARN complete Waiting for 1 child process to exit. CTRL-C to exit immediately.

I did not have the issue with the commas, so that might be related to Windows. But the later error remains.

Here are my globally installed npm packages on Linux Mint:

npm list -g --depth 0
/usr/lib
├── @vue/cli@3.10.0
├── coffee-script@1.12.7
├── css-purge@3.0.4
├── eslint@5.10.0
├── eslint-plugin-html@5.0.0
├── fx@3.1.0
├── generator-botkit@4.0.3
├── gulp@3.9.1
├──  error: ENOENT: no such file or directory, lstat '/home/thomas/workspace/hads
├── imagemin-cli@3.0.0
├── imagemin-gifsicle@5.2.0
├── jshint@2.9.5
├── jslint@0.11.0
├── less@1.3.3
├── npm@6.10.2
├── parcel-bundler@1.7.0
├── prettier@1.11.1
├── purify-css@1.2.5
├── sass@1.1.1
├── strapi@1.6.4
├── svgo@1.0.5
├── swagger@0.7.5
├── toxiproxy-node-client@2.0.6
├── UNMET PEER DEPENDENCY webpack@4.x.x
├── webpack-cli@3.2.3
└── yarn@1.16.0

npm ERR! peer dep missing: webpack@4.x.x, required by webpack-cli@3.2.3
npm ERR! error in /usr/lib/node_modules/hads: ENOENT: no such file or directory, lstat '/home/thomas/workspace/hads'


On the subject of welcoming potential contributors, a link to a short video explaining what lerna is and what it does would be nice. Also a short description of what role have the different packages would help ;)

@janober
Copy link
Member

janober commented Aug 13, 2019

Ah very great that it works now!

If you check out the old version on Linux you actually do not reproduce the previous issue. If you compare the error messages they are totally different. The one you get on Linux is the one I talked about before with that inquirer renamed a type "inquirer.Questions" -> "inquirer.QuestionCollection".
The issue you had before on Windows did prevent TypeScript to even get that far to find that issues. So I somehow guess that if you get back to work tomorrow it maybe still does not work for you directly (even though I really hope I am wrong). If that is the case and you figure out what the problem was please still post the answer her anyway in case somebody else runs into the same issue on this project or another.

Thanks a lot for the feedback! Will add it to my list and improve it the next days.

And btw. welcome to the project. Great to have you on board!

@janober
Copy link
Member

janober commented Aug 14, 2019

Improved now the documentation:
https://github.com/n8n-io/n8n/blob/master/CONTRIBUTING.md

If you think anything else should be added please simply add it. A pair of fresh eyes can judge that for sure much better. Thanks!

@thomasleveil
Copy link
Contributor Author

thomasleveil commented Aug 16, 2019

Just another though regarding contributing (and attracting contributors).

I'm not convinced by the monorepo thing (and need for lerna to handle it). I understand a maintainer of the project would have a better life with the monorepo form and lerna. But on the other hand, wanna-be-contributors are firstly users of the app who are missing a features or are annoyed by a bug. Such wanna-be-contributors won't have much time to learn how to use lerna and to discover how things are organised and linked together in a monorepo, because it won't bring much value to them. That might be a deterrent.

I guess users try to become contributors with the following cases :

  • As a user of the application, I'm stuck because of a bug caused by the code of a Node. I want to quickly hack on a single node code, be able to deploy it quickly to my existing app and go on with my life
  • As a user of the application, I'm missing a functionality on a node configuration (ignore SSL errors on the HTTPRequestNode for instance), I want to quickly grab that node code and be able to deploy it in my currently deployed app
  • As a user of the application and as a Vue developer, I'm glad to discover the n8n editor is in Vue.js, I wish I can improve the UI, but I don't care about the other stuff.

@janober
Copy link
Member

janober commented Aug 16, 2019

Guess there it is how it is with almost all things. Everything has its advantages and disadvantages.
The mono repro is for sure not always great and also caused me very often headaches. However not sure if not using one would really be easier. Because even in the simple case that somebody only wants to make changes to the UI he would still have to have the backend running to connect to it. So that means that user would then suddenly have to check out, install dependencies and start two things. He does not just have to run "npm run dev" once, like right now anymore, which starts everything. And for all changes which do not just involve the frontend but also the backend he would have to commit things separate. Currently, when a bigger change gets made it is one commit where afterward everything thing fits perfectly together. In a multi-repro he would have to commit in up to 5 different repositories (n8n-cli, n8n-core, n8n-editor, n8n-nodes-base, n8n-workflow) separate where the code then also has to be looked at separately. That would not just make it more complicated for things like testing (as I would then have to check each of them out separate then link them together manually) it would also increase the risk that things are not compatible with each other anymore.

So I kind of fear that changing that would replace one problem with an even bigger one. For sure would make it easier for one time contributors but would drive the more involved people crazy.

At least that is what I think would happen. If there is however a good solution for things like mentioned above, I am more than open for any better solution. Is not that I think mono repro is always the way to go but not without reason are more and more projects switching to it and not without reason have also one of the biggest companies in tech mono repositories.

@thomasleveil
Copy link
Contributor Author

Keeping the monorepo, the contributing documentation could welcome new users with shortcuts for frequent contribution cases like :

  • I want to hack the UI
  • I want to hack nodes

giving for each of those cases quick instructions on how to get setup without explaining to much about the other parts of the monorepo project.

@janober
Copy link
Member

janober commented Aug 16, 2019

Yes, that sounds like a very good idea! Also think that could help new contributors a lot and so hopefully attract more.

RicardoE105 pushed a commit that referenced this issue Feb 18, 2022
INT-483: Add missing endpoints to n8n-onfleet node
maxwellharon pushed a commit to maxwellharon/n8n that referenced this issue Mar 16, 2022
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

No branches or pull requests

2 participants