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

feat: add namespace option to generate .d.ts files #9

Merged
merged 8 commits into from
Jul 19, 2023

Conversation

aviemet
Copy link
Contributor

@aviemet aviemet commented Apr 6, 2023

Hello, I'd like to propose that an option for changing the file extension be made available. I took the liberty of making a PR with my suggestion. Would love to know what you think.

This adds a config option to set whether the output file should have a ".ts" or ".d.ts" extension.

Thank you

@aviemet
Copy link
Contributor Author

aviemet commented Apr 10, 2023

So, first off, sorry for the initial PR comment. I meant to only merge to my fork while I was playing around, but accidentally did a PR with no subject or commit message, and then I kinda panicked and edited one in real quick.

What I wanted to propose was an option to generate declaration files rather than ts files. In my project, I'm already using types defined in a global namespace, so having the namespace generated would save a ton of work adding imports to each file. I initially tried just importing the types into a declaration file and exporting them under a namespace declaration, but after a lot of trial and error, my conclusion was that there is an issue with the module system which hinders re-exporting types which themselves were default exports from a file. Sadly, I couldn't find any documentation about this, but changing the default exports to named exports made things work.

Anyway, I tried to make my proposed changes flexible by adding a "namespace" option to the initializer, which if not set generates ts files as normal, but if given a string value, generates d.ts files instead. It would declare a global namespace containing all of the generated types, making them globally available without an import statement. This did require more changes than I anticipated, so I hope that I'm not proposing something which is over-fit to my specific needs.

I'd love to know if this is a feature you feel is worth including.

Thanks again for the great project!

@ElMassimo
Copy link
Owner

Hi Avram!

I like the addition of namespace, I think for certain types of projects it's reasonable to have global types.

For large projects, it's probably better to import types manually, though in those cases it might be viable to trigger generate separately for "areas" of the app, and use a different namespace for each.


I'd rather avoid the indentation setting, would you remove it from this PR?

I'm ok switching the default to tabs, but would prefer a separate PR for that change (maybe any users watching the issue can vote).

@aviemet
Copy link
Contributor Author

aviemet commented Apr 13, 2023

Sure thing, I removed the indentation option. I had added it in part because I happen to enforce tabs for my frontend files and eslint made a bunch of squigglies when peeking at the generated files (which is very much a non-issue).

Also, a quick note about the snapshot tests. I had to change how they are generated because on my Linux build, the "::" character sequence wasn't allowed in a filename. So this also includes a change to the snapshot generation which converts those to "__". I didn't include the updated snapshots because I didn't want to muddy the PR with a bunch of changes, but can do so if you'd like.

@aviemet
Copy link
Contributor Author

aviemet commented Apr 27, 2023

Hello, checking in to see if there's something else I do to make this PR work for you. Let me know, happy to do some refactoring or add some tests.

@ElMassimo
Copy link
Owner

Hi Avram! Life's been busy around here (moved to a new place).

I intend to merge and release this PR, but need to find some time to test it myself. Thanks!

@aviemet
Copy link
Contributor Author

aviemet commented May 23, 2023

Funny, I actually just moved as well, and then immediately went on holiday. I believe this last commit addresses your points above, avoiding breaking changes makes a lot of sense for a minor release.

@aviemet
Copy link
Contributor Author

aviemet commented Jun 23, 2023

Is there anything else I can do to get this ready? I've been using it in my project and haven't had any issues, I'm referencing the branch directly in my Gemfile. I'd really like to get this rolled in to the gem because CircleCI is trying to run the tests in this project during my build process and failing.

@ElMassimo
Copy link
Owner

Hi! This comment is still not addressed, which I think makes this change backwards-incompatible as is.

We'd want no changes to "normal" snapshots, plus all the new ones for "namespaced" tests.

@aviemet
Copy link
Contributor Author

aviemet commented Jul 13, 2023

I've never used Github comments before, so maybe I resolved them incorrectly, but I believe my last commit addressed all of the concerns. Exports are changed back to default, typo is corrected. I reverted the tests to assert default exports as well, and they are passing.

@ElMassimo
Copy link
Owner

ElMassimo commented Jul 13, 2023

If you check this import for example, it will fail under tsc, because there is no Model export in that file.

Exports are changed back to default

Correct, but the imports are still using named imports.

Try reverting all snapshots to main, and re-running tests.

@ElMassimo
Copy link
Owner

Thanks for your contribution Avram!

@ElMassimo ElMassimo changed the title Add option for output file extension feat: add namespace option to generate .d.ts files Jul 19, 2023
@ElMassimo ElMassimo merged commit 6f67b1a into ElMassimo:main Jul 19, 2023
7 checks passed
@ElMassimo
Copy link
Owner

ElMassimo commented Jul 19, 2023

Released in [email protected] 🎉

@aviemet
Copy link
Contributor Author

aviemet commented Jul 20, 2023

Hey, thank you for letting me contribute to this and for your patience while I figured things out. I'm not a professional developer, this is more of a hobby for me, so it was really fun to move through the steps of proposing and contributing a new feature.

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.

None yet

2 participants