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

Provide workarounds for both TS (bad peer-provided types handling) and Glint (mishandling of extensions) issues #292

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

Conversation

NullVoxPopuli
Copy link
Collaborator

This isn't ideal, and I would have PR'd it sooner, but I was kind of hoping that we'd have solutions to these problems by now... but we don't.

To unblock people and to make the blueprint usable for folks wishing to use typescript, we need this work around.

Here are the issues that this PR resolves:

@NullVoxPopuli NullVoxPopuli added the bug Something isn't working label Jun 13, 2024
@NullVoxPopuli NullVoxPopuli changed the title Provide TS workarounds for both TS (bad peer-provided types handling) and Glint (mishandling of extensions) issues Provide workarounds for both TS (bad peer-provided types handling) and Glint (mishandling of extensions) issues Jun 13, 2024
@NullVoxPopuli NullVoxPopuli marked this pull request as ready for review June 13, 2024 14:42
Copy link
Contributor

@BoussonKarel BoussonKarel left a comment

Choose a reason for hiding this comment

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

Applied these changes to my addon (created using blueprint v2.16.0) and they fixed my issues with relative imports of .gts files in my declarations.

Copy link
Contributor

@BoussonKarel BoussonKarel left a comment

Choose a reason for hiding this comment

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

Applied these changes to my addon (created using blueprint v2.16.0) and they fixed my issues with relative imports of .gts files in my declarations.

},
},
<% } %>
],
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this plugin shouldn't be exploded inline into addon's rollup configs 🤔 should it be a new dependency that just provides this? or is it just something that the Embroider addon-dev is providing as a plugin (like the other ones in this file)?

Copy link
Collaborator Author

@NullVoxPopuli NullVoxPopuli Jun 13, 2024

Choose a reason for hiding this comment

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

addon-dev could provide it, if it weren't using tsc to compile (because the way embroider is configured to use tsc right now doesn't actually support await import() in CJS (it gets converted to a require....)).

I would just call it addon.declarations(/* default output "declarations" folder */ ) or something like that.

Copy link
Collaborator Author

@NullVoxPopuli NullVoxPopuli Jun 13, 2024

Choose a reason for hiding this comment

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

I could also just publish a new package that provides these few lines, which is very easy (your first suggestion).

maybe.. declarations() is how it's invoked

(doing this would allow me to author a CJS version of the plugin that does proper await import, for addon-dev to use, if that's what we wanted)

Copy link
Contributor

@ijlee2 ijlee2 Jun 14, 2024

Choose a reason for hiding this comment

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

I'd also like to see the Rollup config remain simple and dependencies to a minimum. If I understood the release notes right, the reference directives won't appear with typescript@5.5?

If so, the blueprint could remain as is (or only be updated to handle importing .gts files), and those who are on typescript@<=5.4 could install fix-bad-declaration-output. Maybe you could first try out the release candidate for 5.5 in v2 addons.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Naw glint is still borked out of the box

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants