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

docs: Add TypeScript to getting started page #379

Merged
merged 4 commits into from
Jan 22, 2024

Conversation

fhildeb
Copy link
Collaborator

@fhildeb fhildeb commented Jan 17, 2024

What kind of change does this PR introduce?

Update the docs

What is the current behavior?

Currently throwing errors for TS users that are unaware of built-in types.

What is the new behavior?

They instantiate their schemas with the ERC725JSONSchema[] type.

@codecov-commenter
Copy link

codecov-commenter commented Jan 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (afd6885) 82.86% compared to head (e5bfe3e) 82.13%.
Report is 11 commits behind head on develop.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #379      +/-   ##
===========================================
- Coverage    82.86%   82.13%   -0.73%     
===========================================
  Files           19       19              
  Lines         1243     1282      +39     
  Branches       286      298      +12     
===========================================
+ Hits          1030     1053      +23     
- Misses         115      126      +11     
- Partials        98      103       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@richtera richtera left a comment

Choose a reason for hiding this comment

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

Probably not a big deal, but taking editing typecript for someone looking for plain javascript might not be awesome.

docs/getting-started.md Show resolved Hide resolved
docs/getting-started.md Show resolved Hide resolved
docs/getting-started.md Outdated Show resolved Hide resolved
@fhildeb fhildeb force-pushed the docs/add-ts-getting-started branch from 31dd83e to 8620183 Compare January 18, 2024 22:04
@fhildeb fhildeb requested a review from CJ42 January 18, 2024 22:04
@CJ42
Copy link
Collaborator

CJ42 commented Jan 18, 2024

I would suggest actually to not go with this change in the docs and instead fix the typing at the schema levels.

Ideally, the schemas should be exported from an index.ts file under the schema/ folder, and have the right typing assigned to them, as:

export const LSP6Schema: ERC725JSONSchema[] = [
    {
        // schema here
    },
    // more objects
]

@richtera
Copy link
Collaborator

But aren't the schemas usually in json files? Making them ts would be better for sure

docs/getting-started.md Outdated Show resolved Hide resolved
@fhildeb
Copy link
Collaborator Author

fhildeb commented Jan 19, 2024

Added all suggestions from above. Do you think this is ready to go now? @CJ42 @Hugoo

docs/getting-started.md Outdated Show resolved Hide resolved
@CJ42 CJ42 merged commit 8cd416b into ERC725Alliance:develop Jan 22, 2024
2 checks passed
### JavaScript

```js
import { ERC725 } require('@erc725/erc725.js');
Copy link
Contributor

Choose a reason for hiding this comment

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

this is weird

Copy link
Contributor

@Hugoo Hugoo left a comment

Choose a reason for hiding this comment

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

I don't understand why we should duplicate the whole code snippet
just to explain that ppl can use : ERC725JSONSchema[]

I would reco to keep only one code snippet and put a comment just where needed

// Typescript type: : ERC725JSONSchema[]

or smtgh

Just to let ppl super easily spot the difference and to have less stuff to maintain

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.

5 participants