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

feat(builtin): flip the default of the strict_visibility flag on the npm and yarn install rules to True #2238

Merged

Conversation

mattem
Copy link
Collaborator

@mattem mattem commented Oct 18, 2020

This is a potential breaking change for uses in 3.0.0

closes #2199

@google-cla google-cla bot added the cla: yes label Oct 18, 2020
@mattem mattem force-pushed the feat/flip_strict_visibility_flag_for_3_0 branch from 829f068 to 16e157d Compare October 18, 2020 14:00
@alexeagle
Copy link
Collaborator

nice, I guess this is failing as expected
ts_project rule //packages/typescript/test/ts_project/a:tsconfig: target '@npm//@babel/parser:parser' is not visible from target '//packages/typescript/test/ts_project/a:tsconfig'.

@mattem
Copy link
Collaborator Author

mattem commented Oct 18, 2020

Yes, these seem like legitimate missing dependencies. Working though them

@jbedard
Copy link
Collaborator

jbedard commented Oct 18, 2020

I was getting an error I think with karma and tmp when I tried enabling this. Not sure if you're seeing that I've? I'll try it again later and confirm if that was the one.

@mattem mattem force-pushed the feat/flip_strict_visibility_flag_for_3_0 branch from 16e157d to ba93012 Compare October 18, 2020 19:37
@mattem
Copy link
Collaborator Author

mattem commented Oct 18, 2020

Hmm, I haven't see that. I did see an issue here of having a dependency on a file:./ reference in the package.json file, which I've fixed in this PR.

@jbedard
Copy link
Collaborator

jbedard commented Oct 18, 2020

_karma_web_test rule //abc:test_wrapped_test: target '@npm//tmp:tmp' is not visible from target '//abc:test_wrapped_test'. Check the visibility declaration of the former target if you think the dependency is legitimate

https://github.com/bazelbuild/rules_nodejs/blob/2.2.2/packages/karma/karma_web_test.bzl#L457-L469

Although that looks like it's actually my fault and this flag caught it? 😄
https://bazelbuild.github.io/rules_nodejs/Karma.html#karma_web_test-peer_deps

@jbedard
Copy link
Collaborator

jbedard commented Oct 18, 2020

A bit odd that @bazel/karma has a tmp peer dep though, is it not? Especially since it's also in the karma/package.json as a dependency... or is there a reason I'm not thinking of? It's also not actually declared as a peerDependency unlike the others listed in that docs link.

(sorry if this is getting off topic for this PR)

@mattem mattem force-pushed the feat/flip_strict_visibility_flag_for_3_0 branch 2 times, most recently from 567955f to 819d7d4 Compare October 19, 2020 13:00
@mattem mattem requested a review from alan-agius4 as a code owner October 19, 2020 13:00
@mattem
Copy link
Collaborator Author

mattem commented Oct 19, 2020

I think the reason it's a peer dep is because it's listed here:
https://github.com/bazelbuild/rules_nodejs/blob/2.2.2/packages/karma/karma_web_test.bzl#L21
Although, I'm not sure what it's used for, and this also assumes the user has an @npm repository

@mattem
Copy link
Collaborator Author

mattem commented Oct 19, 2020

Those integration tests could use the --keep_going flag!

@mattem mattem force-pushed the feat/flip_strict_visibility_flag_for_3_0 branch from 819d7d4 to a1d0e1b Compare October 23, 2020 18:34
@mattem mattem requested a review from mrmeku as a code owner October 23, 2020 18:34
@mattem mattem force-pushed the feat/flip_strict_visibility_flag_for_3_0 branch from a1d0e1b to dd4edb8 Compare October 23, 2020 19:16

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…npm and yarn install rules to True
@mattem mattem force-pushed the feat/flip_strict_visibility_flag_for_3_0 branch from dd4edb8 to 9c883f8 Compare October 23, 2020 19:34
@mattem
Copy link
Collaborator Author

mattem commented Oct 23, 2020

🟢 finally.

This change does mean that users will have to add the deps used by karma_web_test to their own package.json file if they haven't already, these are added by the karma_web_test macro, so it's a little abstracted from the BUILD files a user would see.

@jbedard
Copy link
Collaborator

jbedard commented Oct 24, 2020

This change does mean that users will have to add the deps used by karma_web_test to their own package.json file if they haven't already, these are added by the karma_web_test macro, so it's a little abstracted from the BUILD files a user would see.

That was the issue I came across. I had all of them except tmp which seemed odd to me. All the karma/jasmine peerDependencies sort of make sense, to allow/force the user to define the version they want. But I think the tmp dep is entirely internal to @bazel/karma? I also found that if you don't use the exact version of tmp which @bazel/karam depends on (v0.1.0) then it was crashing for me. It seems like that one should be removed?

@mattem
Copy link
Collaborator Author

mattem commented Oct 24, 2020

tmp is used in the karma concatjs impl to create a unique temp file for the bundled files, but yes it's completely internal to @bazel/karma, not listed as a peerDep, so yeah it's kinda non-intuitive to have that dependency. Perhaps we can just remove it and generate the temp files in the karma package.

@google-cla
Copy link

google-cla bot commented Oct 27, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Oct 27, 2020
@alexeagle
Copy link
Collaborator

I added a commit removing the "tmp" dependency. I don't think it's essential that we cleanup these files ourselves since they go in bazel's TEST_TMPDIR.

@alexeagle
Copy link
Collaborator

@googlebot I consent.

@google-cla google-cla bot added cla: yes and removed cla: no labels Oct 27, 2020
@alexeagle alexeagle merged commit 873bb7f into bazel-contrib:3.x Oct 27, 2020
@jbedard
Copy link
Collaborator

jbedard commented Oct 27, 2020

Can the "tmp" dependency change also go into 2.x?

@mattem mattem deleted the feat/flip_strict_visibility_flag_for_3_0 branch March 4, 2021 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants