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

chore: exclude tools/bundler/node_modules from final package #424

Merged
merged 2 commits into from
Sep 15, 2023

Conversation

Baldinof
Copy link
Contributor

Description

On a fresh install of the package node_modules in the tools/bundler are included while it's only useful when publishing the package.

This will allow to save some space / bandwith when installing this package.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

@Baldinof Baldinof changed the title Exclude tools/bundler/node_modules from final package chore: exclude tools/bundler/node_modules from final package Sep 15, 2023
@smoya
Copy link
Member

smoya commented Sep 15, 2023

Hi @Baldinof! Thanks for submitting this PR!

tools/bundler is an isolated library, that has it's own package.json and could be used outside this project as well.
What do you think about moving that line of gitignore to a new .gitignore file located under tools/bundler dir?

Does it make sense?

@Baldinof
Copy link
Contributor Author

Thanks for you answer @smoya :)

What do you mean by could be used outside this project as well? It can be installed separately?

This PR is to remove the embeddings of the node_modules in tools/bundler from the @asyncapi/specs package tarball.

I don't think adding a .gitignore under tools/bundler.

But maybe tools/bundler is part of the public API of this package but it does not seem to be the case (it's only used to generate files for the release).

@smoya
Copy link
Member

smoya commented Sep 15, 2023

Thanks for you answer @smoya :)

What do you mean by could be used outside this project as well? It can be installed separately?

This PR is to remove the embeddings of the node_modules in tools/bundler from the @asyncapi/specs package tarball.

I don't think adding a .gitignore under tools/bundler.

But maybe tools/bundler is part of the public API of this package but it does not seem to be the case (it's only used to generate files for the release).

As you mentioned, the tools/bundler is expected to be detached from the @asyncapi/specs. It is also isolated in terms of code (all code is located only in tools/bundler), that's why I suggest it needs an independent .gitignore file under /tools/bundler and not to mix with the main .gitignore file in the root.

@Baldinof
Copy link
Contributor Author

The code change is about .npmignore, not .gitignore to ignore files in the npm package tarball.

Node modules are already ignored for git thanks to this

@smoya
Copy link
Member

smoya commented Sep 15, 2023

The code change is about .npmignore, not .gitignore to ignore files in the npm package tarball.

Node modules are already ignored for git thanks to this

omg 🤦 I swear I read .gitignore in the changeset... shame on me! Excuse me for making you waste your time on that matter 🙏

.npmignore Outdated Show resolved Hide resolved
Co-authored-by: Sergio Moya <1083296+smoya@users.noreply.github.com>
@Baldinof
Copy link
Contributor Author

omg 🤦 I swear I read .gitignore in the changeset... shame on me! Excuse me for making you waste your time on that matter 🙏

No worries :)

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link
Member

@smoya smoya left a comment

Choose a reason for hiding this comment

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

LGTM!🙌

@smoya
Copy link
Member

smoya commented Sep 15, 2023

@jonaslagoni could you do a quick check and merge if you agree? Thanks!

Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

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

Thanks @Baldinof 👌

@jonaslagoni
Copy link
Member

/rtm

@asyncapi-bot asyncapi-bot merged commit ff3ab77 into asyncapi:master Sep 15, 2023
11 checks passed
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 6.0.0-next-major-spec.15 🎉

The release is available on:

Your semantic-release bot 📦🚀

@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 6.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants