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

enable IDE auto-completion for the main classes #10618

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

enable IDE auto-completion for the main classes #10618

wants to merge 1 commit into from

Conversation

k-yle
Copy link
Collaborator

@k-yle k-yle commented Dec 20, 2024

With only a few lines of changes + some JSDoc comments, we can significantly improve the DX in this codebase by enabling autocompletion/"intellisense" in most IDEs.

Currently, common classes like context and osmEntity are typed as any, so your IDE can't help you. With this PR, we get autocomplete for most of the class methods:

image

 

We can also reference common classes from JSDoc comments, such as iD.Context or iD.Graph:

/** @param {iD.Context} context */
function whatever(context) { 
  // `context` now has auto-complete 
}

Copy link
Collaborator Author

@k-yle k-yle left a comment

Choose a reason for hiding this comment

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

some comments to explain why this works

@@ -25,7 +25,7 @@ import { utilKeybinding, utilRebind, utilStringQs, utilCleanOsmString } from '..

export function coreContext() {
const dispatch = d3_dispatch('enter', 'exit', 'change');
let context = utilRebind({}, dispatch, 'on');
const context = {};
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This works because JS autocompletion is powered by TypeScript, which is smart enough to infer the type of context when defined as an empty object

proof here

utilRebind can safely be moved to the return statement because this class does not use context.on internally.

@@ -33,7 +37,7 @@ osmEntity.node = osmNode;

osmNode.prototype = Object.create(osmEntity.prototype);

Object.assign(osmNode.prototype, {
const prototype = {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is required so we can use typeof prototype in the JSDoc comment above. No runtime impact.

* @param {...Args} args
* @returns {T & Pick<S, Args>}
*/
export function utilRebind(target, source, ...args) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this had to be refactored to use spread arguments instead of the arguments keyword, because JSDoc doesn't support the arguments keyword.

there's a new unit test to confirm that this is still correctly bound.

@tordans
Copy link
Collaborator

tordans commented Dec 20, 2024

Very nice!

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.

2 participants