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(urijs): compat with non-dom consumers #36261

Merged
merged 1 commit into from Jul 1, 2019

Conversation

Projects
None yet
4 participants
@Akuukis
Copy link
Contributor

commented Jun 18, 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).
  • Provide a URL to documentation or source code which provides context for the suggested changes: urijs is for both browser and nodejs

@Akuukis Akuukis referenced this pull request Jun 18, 2019

Merged

Typescript #298

12 of 14 tasks complete

@typescript-bot typescript-bot added this to Waiting for Reviewers in Pull Request Status Board Jun 18, 2019

@typescript-bot

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

@Akuukis Thank you for submitting this PR!

🔔 @RodneyJT @xt0rted @petejohanson @ljqx @TeamworkGuy2 - 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.

@Akuukis Akuukis force-pushed the Akuukis:urijs branch from 203057f to ef9083a Jun 18, 2019

@typescript-bot

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 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 #36261 diff
Batch compilation
Memory usage 78152680.0 78573320.0 +0.5%
Type count 10076 10114 +0.4%
Assignability cache size 33929 33978 +0.1%
Subtype cache size 15 20 +33.3%
Identity cache size 3 4 +33.3%
Language service
Samples taken 272 538 +97.8%
Identifiers in tests 272 538 +97.8%
getCompletionsAtPosition
    Mean duration (ms) 377.0 390.9 +3.7%
    Median duration (ms) 375.8 388.8 +3.5%
    Mean CV 13.3% 14.6% +10.0%
    Worst duration (ms) 471.3 517.3 +9.8%
    Worst identifier test normalizeQuery
getQuickInfoAtPosition
    Mean duration (ms) 368.7 377.2 +2.3%
    Median duration (ms) 364.7 376.6 +3.3%
    Mean CV 14.3% 15.2% +6.1%
    Worst duration (ms) 499.6 508.9 +1.9%
    Worst identifier createElement URI

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!

@TeamworkGuy2
Copy link
Contributor

left a comment

Does the TypeScript version need to be changed from 2.3 to 3.0 in index.d.ts? Backward compatibility is preferred and these changes look like they should still work with 2.3

@Akuukis

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

@TeamworkGuy2 Yes, /// <reference lib="..." /> comes with TS 3.0. It is used in DOM test file to check whenever monkeypatch is mergable with full DOM typings. I doubt that it impacts types themselves.

@typescript-bot typescript-bot moved this from Waiting for Reviewers to Review in Pull Request Status Board Jun 23, 2019

@typescript-bot

This comment has been minimized.

Copy link
Contributor

commented Jun 23, 2019

After 5 days, no one has reviewed the PR 😞. A maintainer will be reviewing the PR in the next few days and will either merge it or request revisions. Thank you for your patience!

@Akuukis

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2019

Bump! LGTM to merge, do I need to do anything?

@RyanCavanaugh RyanCavanaugh merged commit c4ab682 into DefinitelyTyped:master Jul 1, 2019

3 checks passed

DefinitelyTyped.BenchmarkPR Build #6086 succeeded
Details
DefinitelyTyped.DefinitelyTyped Build #20190618.21 succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Pull Request Status Board automation moved this from Review to Done Jul 1, 2019

@Akuukis Akuukis deleted the Akuukis:urijs branch Jul 1, 2019

morleyzhi added a commit to stellar/js-stellar-sdk that referenced this pull request Jul 2, 2019

fix(ts): compatibility with non-DOM consumers (#357)
Ship minimal monkeypatch of `dom.d.ts` to provide types for non-DOM consumers. Also, the monkeypatch is mergable with the full `dom.d.ts` for DOM consumers. The same has been done at related dependencies:

- DefinitelyTyped/DefinitelyTyped#36259
- DefinitelyTyped/DefinitelyTyped#36261

Also, simplify folder structure.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.