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

Fix mixing access modifiers (e.g. readonly) with @ arguments in constructors #1631

Merged
merged 3 commits into from
Dec 3, 2024

Conversation

edemaine
Copy link
Collaborator

@edemaine edemaine commented Dec 2, 2024

Fixes #1601

Copy link
Contributor

@STRd6 STRd6 left a comment

Choose a reason for hiding this comment

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

We may need to handle the TS output slightly differently since it looks like TS also inserts the assignment: https://www.typescriptlang.org/play/?#code/MYGwhgzhAEBiD29oG8CwAoa1jwHYQBcAnAV2APiIAoiBTMAEzxAE9oAjMIgSmQIAsAlhAB0nItAC8HLgG4AvhnlA

@edemaine
Copy link
Collaborator Author

edemaine commented Dec 2, 2024

I agree that it's redundant, but at least the double assignment is not incorrect...

I could imagine the current behavior of this PR being somewhat helpful in some scenarios:

  • When type checking, you want readonly typing
  • When compiling with js: true, you want the assignment

Probably better would be for Civet to support readonly (and public etc.) in js: true mode, turning into an assignment just like an @ binding. I assume other TypeScript transpilers support this? Yes, esbuild does. Anyway, then we could detect and remove the duplicate, or rather, trigger once when there's an @ binding or has a TypeScript specifier.

@edemaine
Copy link
Collaborator Author

edemaine commented Dec 2, 2024

Probably better would be for Civet to support readonly (and public etc.) in js: true mode, turning into an assignment just like an @ binding

Actually, it looks like we already did this, some time ago.

Here's a fun situation:

class Foo
  @(public @var)

Currently we compile this to:

class Foo {
  constructor(public _var){this.var = _var;}
}

This declares the wrong thing public. But we also don't want to omit the _. I think we need to compile to

class Foo {
  public var
  constructor(_var){this.var = _var;}
}

So I'm tempted to lift public/readonly/etc. to the type declaration, using the existing type declaration lifting. But unlike type lifting, we won't duplicate the modifiers; we'll remove them from constructor.

@edemaine
Copy link
Collaborator Author

edemaine commented Dec 2, 2024

OK, there shouldn't be any duplicate this assignment when mixing access modifiers with @ bindings now, using the alternate compilation above.

@edemaine edemaine changed the title Fix mixing readonly with @ arguments in contructors Fix mixing access modifiers (e.g. readonly) with @ arguments in constructors Dec 2, 2024
Copy link
Contributor

@STRd6 STRd6 left a comment

Choose a reason for hiding this comment

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

Awesome!

@edemaine edemaine merged commit abbf5dc into main Dec 3, 2024
3 of 4 checks passed
@edemaine edemaine deleted the readonly-at branch December 3, 2024 01:57
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.

TypeError when compiling readonly @prop in c'tor
2 participants