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

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

solvingproblemswithtechnology

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
Copy link

CLAassistant commented Aug 14, 2023

CLA assistant check
All committers have signed the CLA.

@solvingproblemswithtechnology solvingproblemswithtechnology marked this pull request as ready for review August 14, 2023 14:21
@Joffcom
Copy link
Member

Joffcom commented Aug 14, 2023

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
Copy link
Author

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
Copy link
Member

Joffcom commented Aug 14, 2023

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
Copy link
Author

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
Copy link

Any news about this PR?

@Joffcom
Copy link
Member

Joffcom commented Nov 15, 2023

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
Copy link

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

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.

4 participants