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(analyzer): ignore and internal jsdoc #89

Closed
wants to merge 1 commit into from

Conversation

bennypowers
Copy link
Member

fixes #21

@bennypowers
Copy link
Member Author

bennypowers commented Jul 10, 2021

It's likely i missed some test cases so please look over

/** @internal */
export const dontIncludeMe = false; // should not be in declarations
/** @ignore */
export const meNeither = false; // should not be in declarations
export const variable = 'var';
/** @ignore */
export class IgnoreMe extends HTMLElement { }
customElements.define("ignore-me", IgnoreMe);
export class IncludeMe extends HTMLElement {
included = 'hello world';
/** @ignore */
sneaky = 'deaky';
/** @internal */
hideMe() {
return '🙈'
}
}
customElements.define("include-me", IncludeMe);
/** @ignore */
var ignoreMePlease = 'haha';
/** @internal */
var excludeMe, andMe = 'something private';
export {
ignoreMePlease,
excludeMe,
andMe,
}

@netlify
Copy link

netlify bot commented Jul 10, 2021

❌ Deploy Preview for custom-elements-manifest-analyzer failed.

🔨 Explore the source changes: cafb94d

🔍 Inspect the deploy log: https://app.netlify.com/sites/custom-elements-manifest-analyzer/deploys/60eab3f0b0a21f0008810cd5

@thepassle
Copy link
Member

we already had an open PR for this that was almost finished here #22

@bennypowers bennypowers mentioned this pull request Jul 11, 2021
@bennypowers
Copy link
Member Author

Ok I covered the case from #22 (comment) as well

@bennypowers
Copy link
Member Author

I think this is ready to go can you check the test cases please?

@thepassle
Copy link
Member

Maybe we should revisit the approach we had in this PR #22, it solved like 99% of the cases with like 5 lines of code. The only case that needed to be fixed was:

class MyEl extends HTMLElement {
  priv2;

  constructor() {
    super();

    /** @ignore */
    this.priv2 = 'hidden';
  }
}

@bennypowers
Copy link
Member Author

PR #22, it solved like 99% of the cases with like 5 lines

Unfortunately the previous PR, though brief, did not cover nearly enough cases to be useful, which is why I closed the previous in favour of this one. The reason this PR has to touch so many files is because in many of the handlers, there's no way to remove the doc in question without causing reference errors down the pipeline, hence the inclusion of null checks at assignment expressions throughout the core, which I think is anyways a good idea.

Test failures on previous branch Screen Shot 2021-07-11 at 12 35 07 Screen Shot 2021-07-11 at 12 35 19 Screen Shot 2021-07-11 at 12 35 23

@thepassle
Copy link
Member

Those test failures dont seem correct? The implementation in the other branch correctly excluded variables, classes, fields. You're correct that it didnt handle custom-element-definitions and @internal yet but that would have been trivial to add

@bennypowers
Copy link
Member Author

ok i'll give it another look maybe we can have it all

@thepassle thepassle closed this Jul 11, 2021
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.

support @ignore tag
2 participants