Skip to content

Conversation

@tusharmath
Copy link

No description provided.

@tusharmath tusharmath changed the title Update README.md Add details about how to use Nov 4, 2022
@someengineer-nordsec
Copy link

This will simply not work if some random package in node_modules uses @types/chrome

Comment on lines +33 to +50

**Steps**

1. Add library as a dependency.
```bash
npm i chrome-types --save-dev
```
3. Update compiler options in `tsconfig.json`

```patch
{
"compilerOptions": {
...
+ "types": ["chrome-types"]
...
},
}
```
Copy link

@jsejcksn jsejcksn Apr 5, 2023

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?

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.

@jpmedley jpmedley requested a review from oliverdunk April 6, 2023 17:26
Copy link
Member

@oliverdunk oliverdunk left a comment

Choose a reason for hiding this comment

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

Thanks for this! I've left a few bits of feedback.

```bash
npm i chrome-types --save-dev
```
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)

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

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

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants