Skip to content

Suppress missing require from externs (reworked) #1366

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

Merged

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

This is a rework of PR #1192 which follows the behaviour of the rest of the class more closely. The check now happens before adding to usages, in visitClassNode, rather than in visitScriptNode, as per @MatrixFrog comment #1192 (comment)

@supersteves
Copy link
Contributor Author

@blickly wrote in #1192 (comment)

Looks live visitNewNode doesn't call maybeAddUsage

@MatrixFrog wrote in #1192 (comment)

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

In fact because this is an ES6 class, the problem was in visitClassNode, which was unreservedly adding a usage for the extends clause.

Line 406 no longer exists, but I think this was maybeAddUsage which is specific to types mentioned in JSDoc, and not the extends clause.

This fix was done sympathetically to visitNewNode; the unit test is unchanged from the original PR.

@supersteves supersteves force-pushed the suppressMissingRequireFromExterns2 branch from eb4650a to d54b644 Compare January 6, 2016 12:24
@ChadKillingsworth
Copy link
Collaborator

This makes sense. I'm surprised I haven't hit this yet.

Node extendClass = classNode.getSecondChild();

// If the superclass is something other than a qualified name, ignore it.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Make the code structure follow this comment with an early return:

if (!extendClass.isQualifiedName()) {
return;
}

// Grab the root superclass namespace.
Node root = ...

In fact then we can probably drop the comment too.

@MatrixFrog
Copy link
Contributor

lgtm other than that. Thank you.

@supersteves supersteves force-pushed the suppressMissingRequireFromExterns2 branch from d54b644 to 7ca9dba Compare January 8, 2016 22:19
@googlebot
Copy link
Collaborator

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@googlebot googlebot added cla: no and removed cla: yes labels Jan 8, 2016
…, e.g. class MyClass extends React.Component (rework of PR 1192 to match rest of class more closely)
@supersteves supersteves force-pushed the suppressMissingRequireFromExterns2 branch from 7ca9dba to ba46b7d Compare January 8, 2016 22:25
@googlebot
Copy link
Collaborator

CLAs look good, thanks!

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

@MatrixFrog Nits done, thanks

MatrixFrog added a commit that referenced this pull request Jan 9, 2016
Suppress missing require from externs (reworked)
@MatrixFrog MatrixFrog merged commit 298f66c into google:master Jan 9, 2016
@supersteves
Copy link
Contributor Author

Thanks!

blickly added a commit that referenced this pull request Jan 11, 2016
Suppress missing require from externs (reworked)

Merge pull request #1381 from Dominator008/indent

Fix format in CheckRequiresForConstructors.java

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=111879244
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