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] Add React plugin declaration #31767

Merged
merged 22 commits into from Jan 3, 2019

Conversation

Projects
None yet
4 participants
@ovidiubute
Copy link
Contributor

ovidiubute commented Dec 29, 2018

  • 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).

If adding a new definition:

  • The package does not already provide its own types, or cannot have its .d.ts files generated via --declaration
@typescript-bot

This comment has been minimized.

Copy link

typescript-bot commented Dec 29, 2018

@ovidiubute Thank you for submitting this PR!

🔔 @szhu - 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.

@ovidiubute

This comment has been minimized.

Copy link
Contributor

ovidiubute commented Dec 29, 2018

For context, @szhu all I did was add a declaration for the linkifyjs react plugin. I've never opened a PR before in this repo so let me know if I screwed up anything 😄

@ovidiubute ovidiubute changed the title Add React plugin to linkifyjs declaration. [linkifyjs] Add React plugin declaration Dec 29, 2018

@szhu
Copy link
Contributor

szhu left a comment

thanks for this PR!

I added some comments that make the style of your code in line with that of my submission -- my goal is simply to make sure that the end result is a linkifyjs declaration that reads well and feels consistent with itself.

I'm fairly new here too, and if any of these suggestions feel weird, just let me know!

(also there are any of the suggestions that you are open to taking but you don't have time, just let me know and I can give you a diff that you can apply.)

"html.d.ts",
"linkifyjs-tests.ts"
]
"files": ["index.d.ts", "html.d.ts", "react.d.ts", "linkifyjs-tests.ts"]
}

This comment has been minimized.

@szhu

szhu Dec 30, 2018

Contributor

Why was this file reformatted?

This comment has been minimized.

@ovidiubute

ovidiubute Dec 31, 2018

Contributor

I have VSCode configured to run Prettier on save, the line width is 80 chars so it reformatted this because it fits within a single line after I added react.d.ts to the list.

This comment has been minimized.

@szhu

szhu Dec 31, 2018

Contributor

Weird, I swear I was using prettier too… this is fine then!

// Definitions: https://github.com/DefinitelyTyped/DefinitelyTyped
// TypeScript Version: 2.1
// TypeScript Version: 2.8

This comment has been minimized.

@szhu

szhu Dec 30, 2018

Contributor

Why did you bump up the minimum TypeScript version?

This comment has been minimized.

@ovidiubute

ovidiubute Dec 31, 2018

Contributor

DefinitelyTyped has a build step that checks whether the dependencies of declarations make sense. Without this bump running npm test throws this error:

Error: linkifyjs depends on react but has a lower required TypeScript version.
    at checkTypeScriptVersions (/home/travis/build/DefinitelyTyped/DefinitelyTyped/node_modules/types-publisher/src/check-parse-results.ts:75:23)
    at Object.checkParseResults [as default] (/home/travis/build/DefinitelyTyped/DefinitelyTyped/node_modules/types-publisher/src/check-parse-results.ts:19:5)
    at <anonymous>

I checked other package declarations and concluded TypeScript needs to be at least at version 2.8 to fix this.

This comment has been minimized.

@szhu

szhu Dec 31, 2018

Contributor

ah because of React. Ok looks legit!


declare class Linkify extends React.Component<LinkifyProps> {}

export { LinkifyProps };

This comment has been minimized.

@szhu

szhu Dec 30, 2018

Contributor

I think you can just do

export interface LinkifyProps {

above but I'm not sure -- let me know if that works!

This comment has been minimized.

@ovidiubute

ovidiubute Dec 31, 2018

Contributor

👍

@@ -1,7 +1,7 @@
import * as React from "react";
import { LinkifyOptions } from "./index";

declare interface LinkifyProps {
interface LinkifyProps {

This comment has been minimized.

@szhu

szhu Dec 30, 2018

Contributor

oh thanks

This comment has been minimized.

@ovidiubute

ovidiubute Dec 31, 2018

Contributor

Thank ESLint for that 😄 Apparently declare is useless in this context.

});

describe("linkifyjs/react", () => {
it("should render a Linkify component", () => {

This comment has been minimized.

@szhu

szhu Dec 30, 2018

Contributor

Regarding using describe and it, do you know if the tests in this repo are actually run? I was under the assumption that they are not, and that these files are only type-checked. I'm not sure though

This comment has been minimized.

@ovidiubute

ovidiubute Dec 31, 2018

Contributor

I took the lead from the react declarations, I am assuming tests are actually run because mocha is a dependency of DefinitelyTyped.

import Linkify from "linkifyjs/react";

declare function describe(desc: string, f: () => void): void;
declare function it(desc: string, f: () => void): void;

// From the docs here: https://soapbox.github.io/linkifyjs/docs/options.html

This comment has been minimized.

@szhu

szhu Dec 30, 2018

Contributor

Since this comment doesn't apply to the section you added, can you move it to just under the describe("linkifyjs/html") line?

describe("linkifyjs/react", () => {
it("should render a Linkify component", () => {
const rootElement = document.createElement("div");
ReactDOM.render(

This comment has been minimized.

@szhu

szhu Dec 30, 2018

Contributor

Can you add tests to test the entire breadth of documented use cases?

You can just rename this file to .tsx and copy all two use cases from this file:
https://soapbox.github.io/linkifyjs/docs/linkify-react.html

Afterwards you can add a comment so the the source of the test is clear! Something like:

 // From the docs here: https://soapbox.github.io/linkifyjs/docs/linkify-react.html

After you add these tests, you will notice that you forgot to declare a prop!

This comment has been minimized.

@ovidiubute

ovidiubute Dec 31, 2018

Contributor

After you add these tests, you will notice that you forgot to declare a prop!

tagName is an optional prop, it defaults to a span element. No real point in testing that if these files are checked by tsc, to be honest, there is no way to screw that up. My bad 🤣

This comment has been minimized.

@ovidiubute

ovidiubute Dec 31, 2018

Contributor

I can add more tests but there is no browser (or jsdom) present so, now that I think of it, I'm really not sure how this code runs at all. I'll do some investigating.

This comment has been minimized.

@ovidiubute

ovidiubute Dec 31, 2018

Contributor

Ah I see what you mean by tagName, thanks 👍

This comment has been minimized.

@ovidiubute

ovidiubute Dec 31, 2018

Contributor

The tests are only compiled, they're not actual unit-tests, you were right. I will remove the calls to it. The describe blocks I think are fine as they add a bit of documentation.

@typescript-bot typescript-bot moved this from Waiting for Reviewers to Needs Author Attention in Pull Request Status Board Dec 30, 2018

@typescript-bot

This comment has been minimized.

Copy link

typescript-bot commented Dec 30, 2018

@ovidiubute One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. Thank you!

@ovidiubute

This comment has been minimized.

Copy link
Contributor

ovidiubute commented Dec 31, 2018

@szhu please have another look

@szhu
Copy link
Contributor

szhu left a comment

thanks for revising! just one clarification

// Definitions: https://github.com/DefinitelyTyped/DefinitelyTyped
// TypeScript Version: 2.1
// TypeScript Version: 2.8

This comment has been minimized.

@szhu

szhu Dec 31, 2018

Contributor

ah because of React. Ok looks legit!

"html.d.ts",
"linkifyjs-tests.ts"
]
"files": ["index.d.ts", "html.d.ts", "react.d.ts", "linkifyjs-tests.ts"]
}

This comment has been minimized.

@szhu

szhu Dec 31, 2018

Contributor

Weird, I swear I was using prettier too… this is fine then!

});

describe("linkifyjs/react", () => {
// From the docs here: https://soapbox.github.io/linkifyjs/docs/linkify-react.html

This comment has been minimized.

@szhu

szhu Dec 31, 2018

Contributor

The point of renaming the file into .tsx was so that you could copy the examples verbatim :p

I was referring to adding this!

/* Usage */

var options: LinkifyOptions = {};
var content = 'For help with GitHub.com, please email support@github.com';
<Linkify tagName="p" options={options}>{content}</Linkify>;

/* Events */

let linkProps = {
  onClick: (event) => {
    if (!confirm('Are you sure you want to leave this page?')) {
       event.preventDefault()
    }
  }
};
<Linkify options={{attributes: linkProps}}>
{null}
</Linkify>;

Please add this in, and also move the // From the docs here: https://soapbox.github.io/linkifyjs/docs/linkify-react.html comment to just above these -- this just makes it easier for people to figure out where the examples are copied from!

This comment has been minimized.

@ovidiubute

ovidiubute Dec 31, 2018

Contributor

Of course, thanks.

This comment has been minimized.

@ovidiubute

ovidiubute Dec 31, 2018

Contributor

So after I added the example I realised the type of LinkifyOptions does not quite match the runtime behavior of the library.

The fact that the value of onClick in this example is (event: any) => void contradicts every other example I could find in their docs (and even unit-tests), so I am not sure how to type that other than use any here:

attributes?: PossiblyFuncOfHrefAndType<{
    [attrName: string]: any;
}> | null;

Let me know if you have a better idea.

This comment has been minimized.

@ovidiubute

ovidiubute Dec 31, 2018

Contributor

I could also do... (but this is pretty bad design in my opinion)

attributes?: PossiblyFuncOfHrefAndType<{
    [attrName: string]: string | ((event: any) => void);
}> | null;

This comment has been minimized.

@ovidiubute

ovidiubute Dec 31, 2018

Contributor

These attributes are fed directly into the React component without any validation so realistically they can be anything. I could use unknown but that requires TypeScript 3.1 I think and would make the declaration more difficult to use.

This comment has been minimized.

@szhu

szhu Dec 31, 2018

Contributor

Also, just a reminder to copy in the JSX tests!

This comment has been minimized.

@ovidiubute

ovidiubute Jan 1, 2019

Contributor

Also, just a reminder to copy in the JSX tests!

I did copy in the JSX tests, not sure what you mean. I can't use the JSX syntax since the project doesn't allow it, and I can't copy the examples verbatim due to linting (no var allowed, no duplicate identifiers, etc.).

This comment has been minimized.

@szhu

szhu Jan 2, 2019

Contributor

The project doesn't allow JSX? How did stuff like this get in here?

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/ink/ink-tests.tsx#L15-L17

Also you can copy it very close to verbatim:

  • Replace var with let
  • Put each example it its own scope
/* Usage */

{
  let options: LinkifyOptions = {};
  let content = 'For help with GitHub.com, please email support@github.com';
  <Linkify tagName="p" options={options}>{content}</Linkify>;
}

/* Events */

{
  let linkProps = {
    onClick: (event) => {
      if (!confirm('Are you sure you want to leave this page?')) {
        event.preventDefault()
      }
    }
  };
  <Linkify options={{attributes: linkProps}}>
  {null}
  </Linkify>;
}

This comment has been minimized.

@ovidiubute

ovidiubute Jan 2, 2019

Contributor

OK no problem, I tried writing JSX but got some errors but they were probably unrelated.

This comment has been minimized.

@ovidiubute

ovidiubute Jan 2, 2019

Contributor

Thanks for the block idea, I don't think I've ever actually used that until today 😄

@typescript-bot

This comment has been minimized.

Copy link

typescript-bot commented Dec 31, 2018

🔔 @szhu - Thanks for your review of this PR! Can you please look at the new code and update your review status if appropriate?

@ovidiubute

This comment has been minimized.

Copy link
Contributor

ovidiubute commented Jan 1, 2019

@szhu I just thought of something -- my latest change effectively means the entire declaration now depends on React, even though technically that's just a plugin of linkifyjs. I'm not sure of the ramifications of that, does it mean that the new version of@types/linkifyjs will depend on @types/react?

@szhu

This comment has been minimized.

Copy link
Contributor

szhu commented Jan 2, 2019

not 100% sure either but it would be weird if @types/linkifyjs pulled in react as a dependency! It makes more sense that it would pull in @types/react at most.

@@ -10,7 +10,8 @@
"typeRoots": ["../"],
"types": [],
"noEmit": true,
"forceConsistentCasingInFileNames": true
"forceConsistentCasingInFileNames": true,
"jsx": "preserve"

This comment has been minimized.

@ovidiubute

ovidiubute Jan 2, 2019

Contributor

This is why I mistakenly thought the repo didn't support JSX.

@szhu

This comment has been minimized.

Copy link
Contributor

szhu commented Jan 2, 2019

this looks great! Sorry I have one more suggestion about how the comments are done. Can you make these changes?

https://gist.github.com/szhu/76394eeeb4d6cc0ff3daf4e62e7b7aad/revisions?diff=split

It doesn't have to be exactly like this, I'm basically trying to make the header hierarchy more clear.

Thanks! other than that it looks great!

@szhu

szhu approved these changes Jan 3, 2019

Copy link
Contributor

szhu left a comment

Thank you for all your work and revisions! This looks great!

@ovidiubute

This comment has been minimized.

Copy link
Contributor

ovidiubute commented Jan 3, 2019

Thanks for the help, @szhu !

@typescript-bot typescript-bot moved this from Needs Author Attention to Check and Merge in Pull Request Status Board Jan 3, 2019

@typescript-bot

This comment has been minimized.

Copy link

typescript-bot commented Jan 3, 2019

A definition owner has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped!

@sheetalkamat sheetalkamat merged commit ba83f54 into DefinitelyTyped:master Jan 3, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@typescript-bot typescript-bot removed this from Check and Merge in Pull Request Status Board Jan 3, 2019

@ovidiubute ovidiubute deleted the ovidiubute:add-linkifyjs-react-plugin-declaration branch Jan 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment