Skip to content

Suppress missing require from externs #1192

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

Closed

Conversation

supersteves
Copy link
Contributor

This is to fix the case where you have React defined globally in externs:

// Externs:

var React = {};

/**
 * @constructor
 * @param {Object} props
 */
React.Component = function(props) {};

// Main code:

class X extends React.Component {
}

Without this PR you get the following warning, which is incorrect since externs don't need to be required as they are global:

'React.Component' used but not goog.require'd

If this is accepted, PR #1147 for suppressing missing requires on class nodes can be deleted.

@@ -182,7 +183,8 @@ private void visitScriptNode(NodeTraversal t) {
(requires == null || (!requires.containsKey(className)
&& !requires.containsKey(outermostClassName)
&& !requires.containsKey(parentNamespace)));
if (notProvidedByConstructors && notProvidedByRequires
boolean notProvidedByExterns = scopeVar==null || !scopeVar.isExtern();
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: spaces around the ==

@MatrixFrog
Copy link
Contributor

I thought the existing var.isExtern() check on line 406 was supposed to make this work. We should figure out why it's not.

@blickly
Copy link
Contributor

blickly commented Oct 7, 2015

Looks live visitNewNode doesn't call maybeAddUsage

@googlebot
Copy link
Collaborator

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

@googlebot googlebot added cla: no and removed cla: yes labels Jan 5, 2016
@supersteves supersteves force-pushed the suppressMissingRequireFromExterns branch from 10077c2 to cdc333b Compare January 5, 2016 14:40
@googlebot
Copy link
Collaborator

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Jan 5, 2016
@supersteves
Copy link
Contributor Author

I'm afraid I broke this PR when I rebased and resolved conflicts with a formatting change. I've committed a fix.

@supersteves
Copy link
Contributor Author

Closing, reworked here:
#1366

@supersteves supersteves closed this Jan 5, 2016
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.

4 participants