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.

Sign up for GitHub

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

Add details about how to use #39

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

Add details about how to use #39

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,21 @@ See [the wiki](https://github.com/GoogleChrome/chrome-types/wiki) for more.
Running the code requires Node 16+ as well as a working version of Python (3 is preferred, but 2.7+ should work) installed on your system.
This has only been tested on Linux and macOS.
Python is used to convert Chromium's internal IDL format to JSON.

**Steps**
Copy link
Member

Choose a reason for hiding this comment

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

Could we put this under a new ### As a dependency header? Steps seems a bit weird since the rest of the text above is talking about how to build.


1. Add library as a dependency.
```bash
npm i chrome-types --save-dev
Copy link

Choose a reason for hiding this comment

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

Suggested change
npm i chrome-types --save-dev
npm install --save-dev chrome-types

It's useful to use/suggest canonical command forms in documentation

```
3. Update compiler options in `tsconfig.json`
Copy link
Member

Choose a reason for hiding this comment

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

Nit: This should be two 2 (or 1 if you want to rely on Markdown handling the increment)


```patch
{
"compilerOptions": {
...
+ "types": ["chrome-types"]
...
},
}
```
Comment on lines +33 to +50
Copy link

@jsejcksn jsejcksn Apr 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It's probably a good idea not to exclude other installed types: types, typeRoots

Edit: After giving this more thought — even including the local node_modules/@types directory might still be exclusive in some cases (e.g. a monorepo), so I'm retracting this suggestion.

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be nice to include either the default node_modules directly, or a note that you need to do that. Would you be ok adding that @tusharmath?

Copy link

Choose a reason for hiding this comment

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

I think it would be nice to include either the default node_modules directly, or a note that you need to do that. Would you be ok adding that @tusharmath?

I agree that a note is appropriate.