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

Update TypeScript to latest #630

Merged
merged 10 commits into from
Aug 27, 2024
Merged

Update TypeScript to latest #630

merged 10 commits into from
Aug 27, 2024

Conversation

simonihmig
Copy link
Contributor

@simonihmig simonihmig commented Jul 4, 2024

Hope this will unblock #623 (comment)

@simonihmig simonihmig changed the title Update TypeScript Update TypeScript to latest Jul 8, 2024
registerHelper('js-string-escape', jsStringEscape);
registerHelper('join', function (list, connector) {
Handlebars.registerHelper('js-string-escape', jsStringEscape);
Handlebars.registerHelper('join', function (list, connector) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The API docs only show that kind of usage, and registerHelper accesses this.. Not sure why it worked before, but as soon as the compilation output from a new TS versions changed this be like (0, handlebars_1.registerHelper)('join', ...), it would lose access to this.

@simonihmig simonihmig marked this pull request as ready for review July 8, 2024 09:36
@simonihmig simonihmig requested review from ef4 and mansona July 8, 2024 09:36
@@ -192,7 +192,7 @@ export default class Analyzer extends Funnel {
try {
transformSync(source, options);
} catch (err) {
if (err.name !== 'SyntaxError') {
if (err instanceof Error && err.name !== 'SyntaxError') {
Copy link
Member

Choose a reason for hiding this comment

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

I understand what you're doing here but I don't like the idea of introducing a subtle behaviour change just to work around types here. I would prefer if you worked around the types like you did later in the PR. Something like (err as unknown as {name: string})? but don't quote me on it 😂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But what's the change in behaviour here? In case of a SyntaxError it will always be an instance of Error, right? And if it's not a SyntaxError, then the if condition will not be hit either way. If feel this is the cleaner way to do this than to override the types, given that TS is actually right here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mansona 👆😏

@ef4 ef4 merged commit b423901 into embroider-build:main Aug 27, 2024
156 checks passed
@github-actions github-actions bot mentioned this pull request Aug 27, 2024
@simonihmig simonihmig deleted the update-ts branch September 4, 2024 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants