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

[linkifyjs] re-write definitions #38900

Merged

Conversation

@G-Rath
Copy link
Contributor

G-Rath commented Oct 5, 2019

Please fill in this template.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Add or edit tests to reflect the change. (Run with npm test.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If adding a new definition:

  • The package does not already provide its own types, or cannot have its .d.ts files generated via --declaration
  • If this is for an NPM package, match the name. If not, do not conflict with the name of an NPM package.
  • Create it with dts-gen --dt, not by basing it on an existing project.
  • Represents shape of module/library correctly
  • tslint.json should be present and it shouldn't have any additional or disabling of rules. Just content as { "extends": "dtslint/dt.json" }. If for reason the some rule need to be disabled, disable it for that line using // tslint:disable-next-line [ruleName] and not for whole package so that the need for disabling can be reviewed.
  • tsconfig.json should have noImplicitAny, noImplicitThis, strictNullChecks, and strictFunctionTypes set to true.

If changing an existing definition:

  • Provide a URL to documentation or source code which provides context for the suggested changes: https://soapbox.github.io/linkifyjs/docs/
  • If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.
  • If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }. If for reason the any rule need to be disabled, disable it for that line using // tslint:disable-next-line [ruleName] and not for whole package so that the need for disabling can be reviewed.

If removing a declaration:

  • If a package was never on DefinitelyTyped, you don't need to do anything. (If you wrote a package and provided types, you don't need to register it with us.)
  • Delete the package's directory.
  • Add it to notNeededPackages.json.

The general gist is that the type definitions for the core are not correct, and the tests don't test the types.

I've re-write the definitions to properly type linkify, but haven't had time to do full testing, due the amount of work improved, and the general state of linkify.

When running the tests locally, I get an error about Dependency @emotion/styled not in whitelist

It's very late here, so I'm about to go to bed - I'll pick this back up in the morning and make any required changes to get the test passing.

@typescript-bot

This comment has been minimized.

Copy link
Contributor

typescript-bot commented Oct 5, 2019

👋 Hi there! I’ve run some quick performance metrics against master and your PR. This is still an experiment, so don’t panic if I say something crazy! I’m still learning how to interpret these metrics.

Let’s review the numbers, shall we?

Comparison details 📊
master #38900 diff
Batch compilation
Memory usage (MiB) 133.6 128.9 -3.6%
Type count 52001 51598 -0.8%
Assignability cache size 52463 52332 -0.2%
Subtype cache size 48 0 -100.0%
Identity cache size 6 3 -50.0%
Language service
Samples taken 166 116 -30.1%
Identifiers in tests 166 116 -30.1%
getCompletionsAtPosition
    Mean duration (ms) 700.6 681.1 -2.8%
    Median duration (ms) 699.1 675.4 -3.4%
    Mean CV 8.0% 5.7% -29.0%
    Worst duration (ms) 817.8 782.8 -4.3%
    Worst identifier Linkify format
getQuickInfoAtPosition
    Mean duration (ms) 631.6 578.1 -8.5%
    Median duration (ms) 625.8 573.5 -8.4%
    Mean CV 9.9% 6.8% -31.6%
    Worst duration (ms) 762.2 646.0 -15.2%
    Worst identifier slice formatHref

It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place.


If you have any questions or comments about me, you can ping @andrewbranch. Have a nice day!

@typescript-bot

This comment has been minimized.

Copy link
Contributor

typescript-bot commented Oct 5, 2019

@G-Rath Thank you for submitting this PR!

🔔 @szhu @ovidiubute - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

@typescript-bot

This comment has been minimized.

Copy link
Contributor

typescript-bot commented Oct 5, 2019

@G-Rath The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

@G-Rath G-Rath force-pushed the G-Rath:improve-linkifyjs-types branch from 51ffec8 to e2184e2 Oct 5, 2019
@G-Rath G-Rath force-pushed the G-Rath:improve-linkifyjs-types branch from e2184e2 to f4627ee Oct 5, 2019
@typescript-bot

This comment has been minimized.

Copy link
Contributor

typescript-bot commented Oct 5, 2019

Updated numbers for you here from 1d081b1:

Comparison details 📊
master #38900 diff
Batch compilation
Memory usage (MiB) 131.1 129.2 -1.4%
Type count 52001 51598 -0.8%
Assignability cache size 52463 52332 -0.2%
Subtype cache size 48 0 -100.0%
Identity cache size 6 3 -50.0%
Language service
Samples taken 166 116 -30.1%
Identifiers in tests 166 116 -30.1%
getCompletionsAtPosition
    Mean duration (ms) 651.4 636.8 -2.2%
    Median duration (ms) 652.6 633.5 -2.9%
    Mean CV 8.3% 6.1% -26.4%
    Worst duration (ms) 763.8 785.9 +2.9%
    Worst identifier attributes value
getQuickInfoAtPosition
    Mean duration (ms) 586.0 540.6 -7.7%
    Median duration (ms) 580.2 535.6 -7.7%
    Mean CV 10.8% 6.6% -39.0%
    Worst duration (ms) 754.2 635.9 -15.7%
    Worst identifier options options

It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place.

@typescript-bot typescript-bot moved this from Needs Author Attention to Waiting for Reviewers in Pull Request Status Board Oct 5, 2019
@typescript-bot typescript-bot moved this from Waiting for Reviewers to Review in Pull Request Status Board Oct 10, 2019
@typescript-bot

This comment has been minimized.

Copy link
Contributor

typescript-bot commented Oct 10, 2019

We've gotten sign-off from a reviewer 👏. A maintainer will soon review this PR and merge it if there are no issues. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for contributing to DefinitelyTyped!

@szhu

This comment has been minimized.

Copy link
Contributor

szhu commented Oct 10, 2019

Hi @G-Rath, I'm the one who added the initial definitions for linkifyjs. I haven't reviewed this yet because (1) there's a lot of changes (it looks good though!) and (2) I've been sick for the past few days. Anyway, just wanted to say hello and also let you know that I do see this PR!!

I'd also welcome a maintainer reviewer of this! I'm guessing the bot has already pinged them?

@G-Rath

This comment has been minimized.

Copy link
Contributor Author

G-Rath commented Oct 10, 2019

@szhu Cheers for the ping. I'm not in any rush about it - I just wanted to get these corrected to reduce the amount of breaking changes for people who might consume the types .

I've used linkify in a couple of my libraries for a while, and so had type definitions for the core for a while, which is why I was surprised when I saw the ones here.

I saw the the two PRs for the types, and read through some of your comments which helped me understand and improve specific areas, such as the tests.

More than happy to answer any questions you might have :)

(and hope you feel better soon!)

@armanio123

This comment has been minimized.

Copy link
Contributor

armanio123 commented Oct 10, 2019

Maintainer here. This is a fairly big change that introduces a bunch of breaking changes but I have very limited knowledge on the library so I'll defer the approval to the original owner. Nonetheless, I have review the type declarations and syntax and lgtm.

Let me know @szhu when review is done and I can help with merging it.

@szhu

This comment has been minimized.

Copy link
Contributor

szhu commented Oct 11, 2019

Hey @G-Rath, one thing that would help me is if you separated this PR into two commits, one of the commits is purely about fixing formatting (single quotes/double quotes, etc.) That way, I can just look at the other commit to review the import parts of the diff.

First, can I ask how you chose the formatting style you used? Is that simply what your editor was set to, or is there some DefinitelyTyped-enforced formatting standard? (There wasn't when I contributed this, but I remember discussions about adding a style guide.)

Anyway, I think your style updates are fine, but it would be great if you could keep them separate the formatting changes from the content changes. Here's a simple way to do that that I typically use:

  1. Checkout the original code:
    git checkout f4627ee~1
  2. Run the formatter you used over all the files in this PR.
  3. Create a commit out of just those changes.
  4. Set the working directory to be after your original changes:
    git checkout f4627ee .
  5. Create a commit out of this. This is the new version of your original commit.
  6. Force push. (GitHub will keep a record of what the old commit was.)
@G-Rath

This comment has been minimized.

Copy link
Contributor Author

G-Rath commented Oct 11, 2019

@szhu The majority of the stylings come from DT - this includes the quotes, simple vs complex array types, and indentation.

What's left over is pretty minimal, and mostly prettier driven. iirc the only conscious styling choice I made was to make the LinkEntityType union chopped, and w/ the ; on a newline - I find that works well for unions of that type, as it makes it easy to adjust it's members and overall treat it as a list.

When I've got a chance I'll look to apply formatting in a commit prior to the type changes, however, I don't think it'll be of much value due to size of changes I've made.

The only formatting change outside of the tsconfig.json is the quotes on the imports - the tsconfig.json itself doesn't require review, since it's controlled by DT.

@G-Rath

This comment has been minimized.

Copy link
Contributor Author

G-Rath commented Nov 1, 2019

@szhu have you managed to have any time to review this?

Sadly I've not had the time to revert the formatting changes, but you should just be able to chuck the old files into prettier to match the current formatting, making it pretty much a non-issue :)

It would be nice to get this merged if possible, given the state of the current typings

@jessetrinity

This comment has been minimized.

Copy link
Contributor

jessetrinity commented Nov 1, 2019

Anyway, I think your style updates are fine, but it would be great if you could keep them separate the formatting changes from the content changes. Here's a simple way to do that that I typically use:

@szhu are you requesting changes before you approve? If so could you start a review so this gets sorted correctly? Otherwise it will probably get merged on YSYL. Thanks!

@szhu

This comment has been minimized.

Copy link
Contributor

szhu commented Nov 1, 2019

Haven't had time, but it's probably fine, I'll just approve it

@szhu
szhu approved these changes Nov 1, 2019
@szhu

This comment has been minimized.

Copy link
Contributor

szhu commented Nov 1, 2019

Sorry I didn't have time to give this a detailed review @G-Rath, but thanks for all the updates to this definition!

@jessetrinity jessetrinity merged commit 5c4d11d into DefinitelyTyped:master Nov 1, 2019
3 checks passed
3 checks passed
DefinitelyTyped.BenchmarkPR Build #17628 succeeded
Details
DefinitelyTyped.DefinitelyTyped Build #20191005.10 succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Pull Request Status Board automation moved this from Other to Done Nov 1, 2019
@typescript-bot

This comment has been minimized.

Copy link
Contributor

typescript-bot commented Nov 1, 2019

I just published @types/linkifyjs@2.1.3 to npm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.