parse-community / parse-server-api-mail-adapter

API Mail Adapter for Parse Server
MIT License
27 stars 18 forks source link

feat: Add TypeScript definitions #90

Closed tbjers closed 1 year ago

tbjers commented 1 year ago

New Pull Request Checklist

Issue Description

Related issue: #89 Closes: #89

Approach

TODOs before merging

parse-github-assistant[bot] commented 1 year ago

I will reformat the title to use the proper commit message syntax.

parse-github-assistant[bot] commented 1 year ago

The branch release can only be set as base branch by members of @parse-community/core-maintainers.

Pull requests are usually opened against the default branch release, which is the working branch. Different repositories may have base branches with different names. If you are sure you need to open this pull request against a different branch, please ask someone from the team mentioned above.

parse-github-assistant[bot] commented 1 year ago

Thanks for opening this pull request!

parse-github-assistant[bot] commented 1 year ago

The branch release can only be set as base branch by members of @parse-community/core-maintainers.

Pull requests are usually opened against the default branch release, which is the working branch. Different repositories may have base branches with different names. If you are sure you need to open this pull request against a different branch, please ask someone from the team mentioned above.

codecov[bot] commented 1 year ago

Codecov Report

All modified lines are covered by tests :white_check_mark:

Comparison is base (0f26c11) 100.00% compared to head (a98a43a) 100.00%.

:exclamation: Current head a98a43a differs from pull request most recent head 320ce40. Consider uploading reports for the commit 320ce40 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## release #90 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 8 8 Lines 546 546 Branches 47 47 ========================================= Hits 546 546 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mtrezza commented 1 year ago

Never mind https://github.com/parse-community/parse-server-api-mail-adapter/pull/90#issuecomment-1756054270, this repository has the release branch as the working branch, hence the comment.

tbjers commented 1 year ago

I am not sure if there is a need to update the documentation with anything. Thoughts, @mtrezza?

mtrezza commented 1 year ago

Wouldn't the typescript definition files normally be committed with the repository? Or how are generated?

tbjers commented 1 year ago

They are automatically generated by the npm build command, so they would be generated at the same time as the rest of the code in lib.

On Tue, Oct 10, 2023, at 18:30, Manuel wrote:

Wouldn't the typescript definition files normally be committed with the repository? Or how are generated?

— Reply to this email directly, view it on GitHub https://github.com/parse-community/parse-server-api-mail-adapter/pull/90#issuecomment-1756356197, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIQW7CXBYE4Z2UXJE4KXOLX6XD6RAVCNFSM6AAAAAA522DXWCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONJWGM2TMMJZG4. You are receiving this because you authored the thread.Message ID: @.***>

mtrezza commented 1 year ago

Got it; regarding docs I don't think there's anything that needs to be added. This doesn't contain any breaking changes for developers who are already using the package, correct?

tbjers commented 1 year ago

Correct.

On Tue, Oct 10, 2023, at 19:03, Manuel wrote:

Got it; regarding docs I don't think there's anything that needs to be added. This will be a minor release without any breaking changes, correct?

— Reply to this email directly, view it on GitHub https://github.com/parse-community/parse-server-api-mail-adapter/pull/90#issuecomment-1756414268, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIQW7D2GQB5GKEBTXTQV3TX6XH5NAVCNFSM6AAAAAA522DXWCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONJWGQYTIMRWHA. You are receiving this because you authored the thread.Message ID: @.***>

mtrezza commented 1 year ago

@sadortun could you review this PR please? I wonder whether TS files should be committed to the repo as the IDE will need them?

sadortun commented 1 year ago

@mtrezza Yes, the file should be added to the repository.

tbjers commented 1 year ago

@sadortun could you review this PR please? I wonder whether TS files should be committed to the repo as the IDE will need them?

cc: @sadortun The files do not need to be committed to the repository because they will be part of the package tarball as they are generated at the same time as everything else in lib/. My reasoning here is that we do not commit the generated code to the repository, either.

sadortun commented 1 year ago

@tbjers @mtrezza Ah, I was referring to tsconfig.json in this PR. And you're referring to .d.ts 😂

If .d.ts are generated by the build step, then you don't need them in the repo.

mtrezza commented 1 year ago

Thanks @sadortun for confirming, and thanks @tbjers for the PR; I'll go ahead and merge.

parseplatformorg commented 1 year ago

🎉 This pull request has been released in version 3.1.0