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

Annotate all internal functions with '@internal' #2183

Open
IvanGoncharov opened this issue Sep 17, 2019 · 8 comments · Fixed by #2205 · May be fixed by #2484
Open

Annotate all internal functions with '@internal' #2183

IvanGoncharov opened this issue Sep 17, 2019 · 8 comments · Fixed by #2205 · May be fixed by #2484
Assignees

Comments

@IvanGoncharov
Copy link
Member

Even though some functions are expored from src/**/*.js files they are private.
The only function exported by src/index.js and src/*/index.js are considered to be part of the public API. Please see:
https://github.com/graphql/graphql-js/blob/master/src/README.md

It would be great to have all private API marked as such, e.g.:

// @internal
export function validateSDL(

Also, it would be great to find all public APIs without JS doc comments.

@wtrocki
Copy link
Contributor

wtrocki commented Sep 17, 2019

Working on it. Removed assignment by mistake

@wtrocki
Copy link
Contributor

wtrocki commented Sep 19, 2019

Spent the entire day on this and doing this by hand is really tricky and prone to errors.
I could do something fast but I would not be content about every change there and verification would be also tricky as well.

How to resolve this problem

My approach for this would be to:

  • Build a unit test/script (after migration to ts it will be ts-lint rule) that will read the entire exported api from the index.
  • Then it will traverse thru the source code and compare individual index.js exports (and individual files)
  • Build diff and print it (later it could just produce a warning

There is the interest of having this kind of check-in TypeScript community (Exported member is not available globally) and we can configure comment format that will disable that as a warning.

Advantage

Advantage of this approach is that:

  • It could be enforced at review time which safes mental effort to review PR's
  • It will be not TypeScript/Flow specific
  • It will reduce the tedious job of asking people to do it for every PR - when introducing new changes
  • Having ability to replicate this fast will mean that there is a lesser chance of doing the same manual job twice when source code will be in typescript.

What it means that it will require much more work initially, but it will be much better for tracking API changes in the future.
@IvanGoncharov

I have seen some babel tools that can help here, but if you guys know any other tools that can help feel free to post it here.

@IvanGoncharov
Copy link
Member Author

@wtrocki tslint is in deprecated state, you can write a custom ESlint and put it in the resources directory.

@wtrocki
Copy link
Contributor

wtrocki commented Sep 19, 2019

Yes. I meant ESLint. For the moment I just have node script that need to print out method names so I can find them and manually add comment

@wtrocki
Copy link
Contributor

wtrocki commented Sep 22, 2019

Got some progress on this but still not ready for PR. I will be off next week so hopefuly I will manage to create PR next next week

craicoverflow pushed a commit to craicoverflow/graphql-js that referenced this issue Sep 27, 2019
craicoverflow pushed a commit to craicoverflow/graphql-js that referenced this issue Sep 27, 2019
craicoverflow pushed a commit to craicoverflow/graphql-js that referenced this issue Sep 30, 2019
@IvanGoncharov IvanGoncharov reopened this Oct 1, 2019
@IvanGoncharov
Copy link
Member Author

@craicoverflow Thanks a lot for #2205 I merged it in but I will keep this issue open to tracking the development of ESLint rule.
Another great subtask would be to check that all public functions have doc comments.

@nveenjain
Copy link

@IvanGoncharov , related to the development of ESLint Rule, I was thinking we can do it as 2 step process:-

  1. Get list of exported functions/classes from the index files...
  2. Compare exported function/class name in the current file with that of the exported functions/classes from the index and generate an error if internal JSDoc comment is not present...

We can execute the first step through babel plugin to get list of all the exported variables from the index in current subdirectory of Graphql...
For the second step we can search for the value of the name of function/class in the list generated... If not present, current function must have internal annotated...

This might cause a problem when 2 different files export the function/class having same name... We can tackle this problem in 2 ways:-

  1. We must ensure that this never happens and exported names are always different for different files atleast under same sub-directory in graphql (This will be easier to code)
  2. Add the source too in the list generated and use it in second step... (A little bit complicated to code)

Can I take up this issue, if yes, which approach should I follow?

@nveenjain
Copy link

nveenjain commented Mar 9, 2020

I created a babel-plugin for the same since we are using only ExportNamedDeclarations, this plugin only extracts the list of exported variables with ExportNamedDeclarations... This means that code like

 export let name1, name2=5, nameN;

is not supported, and name1, name2, nameN will not be output of the plugin... However this can be modified if required...
You can check the plugin live at:- https://astexplorer.net/#/gist/01dc219b927b6a72efc1ca00a92fa0be/35b47cf3831059793f993bbd3a4d71b140759972

You can check the console to get the array of exported names/functions...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants