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

[Experiment] feat(17227): Support @abstract JSDoc tag #42186

Closed
wants to merge 0 commits into from

Conversation

a-tarasyuk
Copy link
Contributor

Fixes #17227

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Jan 3, 2021
@a-tarasyuk a-tarasyuk force-pushed the feat/17227 branch 2 times, most recently from 960d619 to be19784 Compare January 3, 2021 15:52
src/compiler/checker.ts Outdated Show resolved Hide resolved
src/compiler/checker.ts Outdated Show resolved Hide resolved
tests/baselines/reference/jsdocAbstract2.js Outdated Show resolved Hide resolved
tests/cases/conformance/jsdoc/jsdocAbstract2.ts Outdated Show resolved Hide resolved
tests/cases/conformance/jsdoc/jsdocAbstract4.ts Outdated Show resolved Hide resolved
tests/cases/conformance/jsdoc/jsdocAbstract4.ts Outdated Show resolved Hide resolved
@a-tarasyuk a-tarasyuk force-pushed the feat/17227 branch 2 times, most recently from 1f3599d to 9dfbcfd Compare January 3, 2021 20:15
tests/baselines/reference/jsdocAbstract2.js Outdated Show resolved Hide resolved
tests/cases/conformance/jsdoc/jsdocAbstract2.ts Outdated Show resolved Hide resolved
tests/cases/conformance/jsdoc/jsdocAbstract4.ts Outdated Show resolved Hide resolved
@a-tarasyuk a-tarasyuk closed this Jan 4, 2021
@a-tarasyuk a-tarasyuk reopened this Jan 4, 2021
@a-tarasyuk a-tarasyuk changed the title feat(17227): Support @abstract JSDoc tag [Experiment] feat(17227): Support @abstract JSDoc tag Jan 4, 2021
@ajafff
Copy link
Contributor

ajafff commented Jan 7, 2021

We also need to determine whether legacy class syntax should be recognized:

/**
 * @class
 * @abstract
 */
function Legacy() {
  /**
   * @type {string}
   * @abstract
   */
  this.prop;
}

/** @abstract */
Legacy.prototype.doStuff = function() {}

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Here's what I think is needed to finish this feature:

  1. Survey existing code to see whether requiring @abstract methods only to occur in @abstract classes would result in lots of errors.
  2. Figure out how serialisation code results in issuing errors, then move the error earlier.
  3. Add test cases.
  4. Restrict what's allowed in the body of abstract methods.

tests/baselines/reference/jsdocAbstract2.errors.txt Outdated Show resolved Hide resolved
tests/cases/conformance/jsdoc/jsdocAbstract4.ts Outdated Show resolved Hide resolved
tests/baselines/reference/jsdocAbstract2.js Outdated Show resolved Hide resolved
tests/cases/conformance/jsdoc/jsdocAbstract3.ts Outdated Show resolved Hide resolved
tests/baselines/reference/jsdocAbstract4.js Outdated Show resolved Hide resolved
src/compiler/checker.ts Outdated Show resolved Hide resolved
src/compiler/checker.ts Outdated Show resolved Hide resolved
@sandersn
Copy link
Member

Here are a couple of observations based on my survey of existing usage:

  1. The majority of existing @abstract on methods will get an error from the code currently in checkJSDocAbstractTag.
  2. There's not much usage, though, so it wouldn't inconvenience many people.
  3. Because there's so little usage already, I'm not sure this feature is worth it.

To accept this I think we'd need buyin from an existing user who wants this feature and agrees to test that it meets their needs. Looking at usage rates, only webpack or discord.js make sense to me.

@a-tarasyuk a-tarasyuk force-pushed the feat/17227 branch 2 times, most recently from 4c3cc7d to aa4e38e Compare January 13, 2021 18:00
@a-tarasyuk a-tarasyuk closed this Feb 16, 2021
@sandersn
Copy link
Member

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 16, 2021

Heya @sandersn, I've started to run the tarball bundle task on this PR at aa4e38e. You can monitor the build here.

@sandersn sandersn reopened this Mar 16, 2021
@sandersn
Copy link
Member

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 16, 2021

Heya @sandersn, I've started to run the tarball bundle task on this PR at 15b01fd. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 16, 2021

Hey @sandersn, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/98652/artifacts?artifactName=tgz&fileId=606CC13BB0AC03ADD769068E6B431AFC3E8D76BC07CDFE3667DFE540A6500AC002&fileName=/typescript-4.3.0-insiders.20210316.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

@sandersn sandersn added the Experiment A fork with an experimental idea which might not make it into master label Apr 5, 2021
@andrewbranch
Copy link
Member

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 21, 2021

Heya @andrewbranch, I've started to run the tarball bundle task on this PR at dfeab9e. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 21, 2021

Hey @andrewbranch, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/107205/artifacts?artifactName=tgz&fileId=54C7D117B51B8A5AF5DBD35A138D051B9C0C44266E24F9ED844D8B25ED71229C02&fileName=/typescript-4.4.0-insiders.20210721.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

@trusktr
Copy link
Contributor

trusktr commented Jan 11, 2022

@a-tarasyuk Can you please restore the content of the PR (it seems to have been cleared with your last force push) so that someone else can give this a shot on top of the work you made?

@trusktr
Copy link
Contributor

trusktr commented Jan 11, 2022

Is there some way to clone/pull the deleted commit, dfeab9e? It appears to be visible on GitHub, but there's a note saying it does not exist in the repo. Same with a-tarasyuk@dfeab9e. I suppose I can see the changes, and manually apply them to the files.

@forivall
Copy link

forivall commented Apr 3, 2024

@trusktr idk you're still wondering, but it can be fetched by git fetch origin dfeab9e4522e64e17174cae3ef700b410f6e4352 && git branch feat/17227 FETCH_HEAD - this will restore the branch into your local checkout.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Experiment A fork with an experimental idea which might not make it into master For Backlog Bug PRs that fix a backlog bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support @abstract JSDoc tag
7 participants