Skip to content

improve bazel builds for node#2499

Merged
anonrig merged 1 commit intomainfrom
yagiz/improve-node-build
Aug 8, 2024
Merged

improve bazel builds for node#2499
anonrig merged 1 commit intomainfrom
yagiz/improve-node-build

Conversation

@anonrig
Copy link
Copy Markdown
Contributor

@anonrig anonrig commented Aug 8, 2024

Moves Node declarations to a separate file to improve caching.

@anonrig anonrig requested review from a team as code owners August 8, 2024 17:40
@anonrig anonrig requested review from jasnell and vickykont August 8, 2024 17:40
@anonrig anonrig force-pushed the yagiz/improve-node-build branch from 187d13b to 3300551 Compare August 8, 2024 17:41
@anonrig anonrig requested review from fhanau and mikea August 8, 2024 17:41
Copy link
Copy Markdown
Contributor

@fhanau fhanau left a comment

Choose a reason for hiding this comment

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

Thank you for doing this, I wasn't aware that separating node from io was possible! Left a number of suggestions here but these are largely stylistic concerns and this will be a win either way. Make sure to open a branch on the downstream repo before merging, it is possible that it will require a few changes now that the io target is smaller.

Copy link
Copy Markdown
Contributor

@mikea mikea left a comment

Choose a reason for hiding this comment

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

thanks for doing this, @fhanau has left excellent suggestions.

Copy link
Copy Markdown
Collaborator

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM with Felix's suggestions

@anonrig anonrig force-pushed the yagiz/improve-node-build branch from 3300551 to c4d0843 Compare August 8, 2024 21:01
@anonrig anonrig force-pushed the yagiz/improve-node-build branch from c4d0843 to 465a6ef Compare August 8, 2024 21:02
@anonrig anonrig merged commit 0685eec into main Aug 8, 2024
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