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

Types for ember-cookies #865

Merged
merged 3 commits into from
Oct 13, 2023
Merged

Types for ember-cookies #865

merged 3 commits into from
Oct 13, 2023

Conversation

And012
Copy link

@And012 And012 commented Oct 12, 2023

No description provided.

@BobrImperator
Copy link
Contributor

Hi, thanks for the PR.

It seems like the package.json is missing types configuration key, which should point to the index.d.ts file. See https://github.com/mainmatter/ember-simple-auth/pull/2514/files#diff-4224f4bb715c8d1383afd410185b784b261a6dec77e2ebdbe329477d4f52d013R15

@@ -43,6 +43,7 @@
"@babel/plugin-proposal-decorators": "7.23.0",
"@embroider/addon-dev": "4.1.1",
"@rollup/plugin-babel": "6.0.4",
"@types/ember": "^4.0.6",
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like this dependency is extraneous, since the index.d.ts is not extending any of the Ember types?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the comment make sense. I will update the pr

@And012 And012 changed the title Types for clearAllCookies Types for ember-cookies Oct 12, 2023
@BobrImperator
Copy link
Contributor

The PR looks good, there are 2 things missing though before we can merge it.

  • We might want to add @ember/types back for autocomplete while modifying the index.d.ts.
    Previously it wasn't needed because the PR didn't use any of the Ember objects, but now it does.

  • We should make sure that the index.d.ts is shipped to the npm registry.
    Currently it's missing from the tarball that is produced by npm publish, see:
    image

You can manually test this by running these commands:

  • npm publish --dry-run, it'll run all the build commands as if it was about to publish the package to the registry.
  • npm pack, it'll produce a tarball like on the screenshot above.

Copy link
Contributor

@BobrImperator BobrImperator left a comment

Choose a reason for hiding this comment

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

Looks good to me 🙌

@BobrImperator BobrImperator merged commit a30d843 into mainmatter:master Oct 13, 2023
12 of 13 checks passed
@BobrImperator
Copy link
Contributor

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

Successfully merging this pull request may close these issues.

None yet

2 participants