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

Fix transitive propagation of npm deps #280

Closed

Conversation

@mvukov
Copy link
Author

mvukov commented Dec 20, 2022

@alexeagle @thesayyn PTAL.

@thesayyn
Copy link
Member

@gregmagolan as he knows the JSInfo provider better.

@gregmagolan
Copy link
Member

Adding this my list of OSS bug squashing for later this week

@alexeagle
Copy link
Member

We just need to be careful we don't re-introduce eager type-checking or cascading rebuilds.

@gregmagolan
Copy link
Member

I'll keep it in mind. We did add analysis test cases to cover that now.

@mvukov
Copy link
Author

mvukov commented Dec 21, 2022

Anything I can help with?

@flolu
Copy link

flolu commented Jan 9, 2023

@gregmagolan @thesayyn @alexeagle why isn't that merged yet? rules_js#677 still persists

@gregmagolan
Copy link
Member

gregmagolan commented Jan 9, 2023

Thanks for the ping. I didn't have time to get to this before the Christmas break. Will look soon. This needs a bit of time to understand the side-effects of this change. In the meantime, it's a small patch to carry downstream if you need this change.

@flolu
Copy link

flolu commented Jan 9, 2023

@gregmagolan awesome, thanks! How would I apply this patch to ts_project.bzl and use it for my project in the meantime?

@gregmagolan
Copy link
Member

gregmagolan commented Jan 9, 2023

It's a very common practice to patch Bazel rules with http_archive in your WORKSPACE. Even rule sets sometimes patch other rule sets. For example, https://github.com/bazelbuild/rules_go/blob/master/go/private/repositories.bzl#L78

http_archive docs: https://bazel.build/rules/lib/repo/http#http_archive-patches

@mvukov
Copy link
Author

mvukov commented Jan 23, 2023

@gregmagolan Friendly ping :) PTAL when you find some time.

@gregmagolan
Copy link
Member

Turns out that @jbedard and I have been discussing the underlying issue that the PR address already. I go into the details of the problem in this comment #268 (comment). The principled fix for this starts with a PR in rules_js to introduce a dev_npm_deps attribute. Closing this PR as the fix will take a different shape once the change in rules_js lands. Will leave #268 open.

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.

5 participants