Skip to content

Allow to suppress import in typedef JSDoc parse error. #3323

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
wants to merge 1 commit into from

Conversation

zavr-1
Copy link
Contributor

@zavr-1 zavr-1 commented Apr 8, 2019

This is to allow writing stuff like because everyone is using import since VSCode has enabled it. It'd be great to have this in Closure as well otherwise types have to be imported via externs and that stops advanced mangling.

/**
 * @suppress {nonStandardJsDocs}
 * @typedef {import('restream').Rule} restream.Rule
 */

@googlebot
Copy link
Collaborator

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@brad4d
Copy link
Contributor

brad4d commented Apr 9, 2019

Created internal issue http://b/130218393

@brad4d brad4d added the internal-issue-created An internal Google issue has been created to track this GitHub issue label Apr 9, 2019
@brad4d
Copy link
Contributor

brad4d commented Apr 9, 2019

I believe this PR is intended as a step toward resolving #3041

@brad4d
Copy link
Contributor

brad4d commented Apr 9, 2019

@zavr-1 you'll need to accept the CLA before we can go any further here.

@zavr-1
Copy link
Contributor Author

zavr-1 commented Apr 9, 2019

@googlebot @brad4d I signed it!

@brad4d
Copy link
Contributor

brad4d commented Apr 10, 2019

@zavr-1 maybe you should try to sign the CLA again, since the check for that is still failing?

@brad4d
Copy link
Contributor

brad4d commented Apr 10, 2019

@zavr-1 or maybe just double-check the instructions for CLA signing. Looks like there are a few things that could be out of whack (email address mismatch, etc.)
Thanks.

@zavr-1
Copy link
Contributor Author

zavr-1 commented Apr 10, 2019

@brad4d hi i've signed it yesterday but it got voided as I'd signed with wrong title, today I've changed the title to the correct one so I suppose just waiting for it to be checked again.

@brad4d
Copy link
Contributor

brad4d commented Apr 11, 2019

@zavr-1 I took a look at a view of the CLA state I get as a Googler for this PR.

It's the first time I've ever needed to do that, so I'm not really familiar with it.
However, it looks like there's some kind of mismatch between the email address recorded in the GitHub commits and the PR sender.

Basically it seems to say that @zavr-1 is the PR sender, but with no known email address.
While the email of the GitHub committer is [REDACTED], which has no known GitHub user name.

FTR, I the UI also had a "Force Rescan" button, which I clicked, but resulted in no change of state.

@googlebot
Copy link
Collaborator

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@zavr-1
Copy link
Contributor Author

zavr-1 commented Apr 13, 2019

@brad4d ok just as I commented the bot got it (thanks @googlebot green is 💚 beautiful)! Thanks for looking into it btw. Also this is why I need this PR https://github.com/artdecocode/typal/ (cmd-f to @suppress to get straight to the point).
all best

@brad4d brad4d self-assigned this Apr 15, 2019
@brad4d
Copy link
Contributor

brad4d commented Apr 16, 2019

PR imported and sent for testing.

@brad4d
Copy link
Contributor

brad4d commented Apr 19, 2019

minimal testing was successful
sending for "test all the things" testing
FYI, I needed to do a minor fix in JsDocInfoParser.java.

typeNode.getString() == "import" => "import".equals(typeNode.getString())

I haven't bothered to push that back to the PR branch, though.
Some permissions errors I encountered when importing the PR probably mean I don't have permissions needed to do that anyway. (Before you ask, I don't remember the error details now, just that my standard procedure for importing didn't work in some way.)

@brad4d
Copy link
Contributor

brad4d commented Apr 22, 2019

I'm submitting the internal change today.
I expect it to be pushed to GitHub tomorrow.

@zavr-1
BTW, I'm not sure this will really solve your original problem.
Errors with parsing JSDoc that get caught at code-parsing time rather than later can't really be suppressed because they happen at a point in the compiler where the suppression logic doesn't get invoked.
I'm not sure this will be a problem for you, but I thought I should at least warn you it could be an issue.

@tjgq tjgq closed this in 0fa50bc Apr 24, 2019
@zavr-1
Copy link
Contributor Author

zavr-1 commented Apr 24, 2019

@brad4d thanks for you time into working this pr through. Also I've had successfully suppressed these errors in many packages now so not sure what you say is correct, sorry. I've been testing this feature on the custom build quite extensively. nonStandardJsDocs always worked perfectly well with all the

/**
 * @suppress {nonStandardJsDocs}
 * @typedef {import('test').Test} test.Test
 */

I'll be looking into making the necessary changes to actually enable the import statements in due time. It would amazing if all it got down to resolving the type names to strings like Test$$module$tt (for import Test from './tt') but it's probably not as easy, so I'll try to understand the JSDoc code and its relation to the actual "types map" better.

@brad4d
Copy link
Contributor

brad4d commented Apr 24, 2019

@zavr-1 I'm glad to hear it works for you.
I didn't know for certain it wouldn't, just thought you might hit the problem I'd seen before.

Re: closure-compiler support for dynamic import the tracking issue is #2770

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes internal-issue-created An internal Google issue has been created to track this GitHub issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants