n8n-io / n8n-nodes-starter

Example starter module for custom n8n nodes.
MIT License
190 stars 128 forks source link

Update packages and remove gulp dependecy to fix all warnings. Add initial docker and docker-compose support for development. #40

Open solvingproblemswithtechnology opened 1 year ago

solvingproblemswithtechnology commented 1 year ago

Hi!

I've been creating some nodes, but I found several problems when using the starter. First, the old dependencies and all the warnings and security issues with gulp. I fixed them and simplified the setup of the project a bit by using Docker and Docker compose.

It's still missing an improvement for using volumes for mounting the custom modules so you don't need to rebuild everytime and you can just watch, but WIP.

CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

Joffcom commented 1 year ago

Hey @solvingproblemswithtechnology,

Which security issues are you referring to with gulp? I like the idea of the container for testing although the shell script might need to be replaced with a more friendly x-platform alternative.

solvingproblemswithtechnology commented 1 year ago

Hi @Joffcom !

Well, Gulp last update was 4 years ago and npm wasn't happy about it. I replaced its usage with rimraf and copyfiles which are simpler and had no issues. Also bumped the rest of the packages to start fresh on the latest versions.

The shell script is being run from the container, so it doesn't need to be crossplatform. Sorry if the name confused you, I can move it a build folder or change the name, any preference?

I also forgot to add that to run it you just need to run docker compose up -d, the standard command. I can add a package.json command also if you like.

Joffcom commented 1 year ago

Hey @solvingproblemswithtechnology,

Perfect thanks, I think maybe a better solution would be to bring this project more inline with our main project when it comes to dev dependencies. Also swapping to pnpm like an earlier PR has.

I missed that the docker image was using the script, That should probably be in a build folder or similar. It would also be worth using node-18 instead of 19 to avoid any possible issues there.

Annoyingly I made 2 community nodes over the weekend and I would have given this a go to make sure it works and to check what needs updating in the docs.

solvingproblemswithtechnology commented 1 year ago

Hey @Joffcom

Maybe we can work on that on following PRs. The scope of this one was to fix all npm warnings and bring basic docker support. I think that's a good first step, and the only one I have knowledge to contribute. Since your main projects is way bigger and has different needs, I'm not sure about bringing all that extra complexity for this basic template. This template is for people coming with no context, not for people with a lot of knowledge of your main project.

Changed! I called it docker/ like in your main project and used node 18.

I tested all this setup while building my own Azure ServiceBus nodes, since the AMQP ones are lacking some important bits and not trully working with ASB. Which btw, how can I send them to you? David Roberts said it would be nice to take a look at them, but didn't tell me how 😅

kimtiago commented 10 months ago

Any news about this PR?

Joffcom commented 10 months ago

Hey @kimtiago,

Nothing yet I have not had a chance to look at it again but this PR shouldn't be blocking anyone from making nodes the current repo has been used for a lot of community nodes now.

I will make sure I get to this very soon though as it is looking better and I can see why some may prefer an approach like this.

kimtiago commented 10 months ago

Understood, but I just want only c1320ac commit to update dependencies