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

Made it all typescripty #22

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

Made it all typescripty #22

wants to merge 19 commits into from

Conversation

molebox
Copy link

@molebox molebox commented Sep 14, 2021

Hey Jason! I took a swing at converting this package to typescript and it seems to work nice for me on my fork. Thought i would open a PR and see if you were interested in any in any of this code. Im no TS pro so there might be things that should be tightened up. 😊

@molebox
Copy link
Author

molebox commented Sep 14, 2021

This is getting out of hand with these commits. Closing this PR until its sorted 😅

@molebox molebox closed this Sep 14, 2021
@molebox molebox reopened this Sep 14, 2021
@molebox
Copy link
Author

molebox commented Sep 14, 2021

I fixed it! Sorry about all the commits 😬

Copy link
Owner

@jlengstorf jlengstorf left a comment

Choose a reason for hiding this comment

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

thank you for tackling this! it looks like some generated files got checked in, but overall I think this looks solid. I have one question about the dist target and package changes that might cause issues with Toast, but I'll try that out and see how it goes

could you pull the generated files out, please?

src/index.js Outdated Show resolved Hide resolved
src/index.d.ts Outdated Show resolved Hide resolved
dist/index.js Outdated Show resolved Hide resolved
dist/index.d.ts Outdated Show resolved Hide resolved
.gitignore Show resolved Hide resolved
@@ -16,17 +11,16 @@
"scripts": {
"build:cjs": "tsc src/index.ts --outDir . -d",
"build:esm": "cpy ./src/index.ts . --rename=index.mjs",
"build": "yarn build:cjs && yarn build:esm",
"build": "tsc",
Copy link
Owner

Choose a reason for hiding this comment

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

I think this might cause issues with Toast due to ESM/CJS compat problems. I'll try to pull this and try it out to see

@molebox
Copy link
Author

molebox commented Sep 15, 2021

thank you for tackling this! it looks like some generated files got checked in, but overall I think this looks solid. I have one question about the dist target and package changes that might cause issues with Toast, but I'll try that out and see how it goes

could you pull the generated files out, please?

Thanks! Ive not forked an npm package before so i apologize for the many commits after the fact. This was more confusing than i thought it would be.

Ive added the dist folder to gitignore, but added it to the files array so its not included in the repo. This of course means that i cant test this locally on my repo with:

"@jlengstorf/get-share-image": "https://github.com/molebox/get-share-image.git",

As thats pulling the branch without the dist, so no types and module found. But this should be correct now. As for the removal of your cjs/esm script, sorry im not sure how to proceed there. If this has just created a bunch of work for you to do then of course feel free to close this PR 😅

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.

2 participants