Skip to content

Conversation

@sandersn
Copy link
Contributor

@sandersn sandersn commented Jul 23, 2025

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and discussions and couldn’t find anything or linked relevant results below
  • I made sure the docs are up to date
  • I included tests (or that’s not needed)

Description of changes

I am testing Typescript 7's JS support, which I've largely rewritten during the switch to Go. The new code doesn't support old, Closure-derived syntax, such as uninitialised class property declarations, which VFileMessage uses.

There are multiple ways to fix this. I went for the one that changes only types, with no runtime effects.

  1. Move the optional properties to an interface type in a separate .d.ts file and merge that with Error so Typescript thinks they are there without changing the runtime shape of VFileMessage. There's no JS syntax for this so I had to use Error instead of VFileMessage--but node's error already has actual and expected.
  2. Declare the properties as class properties in the body of the class. This splits the properties up in a non-obvious way though, and it changes the shape of the class at runtime.
  3. Add undefined or '' initialisers to the optional properties. This also changes the shape of the class at runtime.

Other options are:

  1. Do nothing, don't update to TS7.
  2. Switch to .ts, add a build step to strip types.
  3. Add a string index signature to VFileMessage, indicating that it can have any custom properties added to it, without enumerating those properties. This would also have to be on the Error interface since there's no syntax for this in JS.

I don't know how VFileMessage is used; having a consistent runtime shape by virtue of using class property declarations or initialising all properties to undefined generally means: faster performance, but more memory usage. This only matters in cases where people are creating millions of these objects per second. I avoided that change this time, though, because the documentation explicitly mentions that the properties I changed are optional, and the code has some clever manipulation of the prototype to avoid setting file on every object.

Because TS7 is quite a way off, I don't know whether you'll want to take this PR. I created it to see how hard it would be to update popular JS code that uses TS for checking.

Edit: the first commit uses method (3), which is a smaller code change and a bigger runtime change.

sandersn added 2 commits July 23, 2025 10:23
I am testing Typescript 7's JS support, which I've largely rewritten
during the switch to Go. The new code doesn't support old,
Closure-derived syntax, such as uninitialised class property
declarations, which VFileMessage uses.

I made the smallest syntactic change to fix this, which is to initialise
all the optional properties declared this way with `undefined`. The
other two ways I thought of are:

1. Declare these properties as class properties in the body of the
class. This splits the properties up in a non-obvious way though.
2. Declare an interface type in a separate .d.ts file and merge that with VFileMessage so
Typescript thinks they are there without changing the runtime shape of
VFileMessage. I can't think of the JS syntax to make this merge happen,
though.

Other options are:

3. Do nothing, don't update to TS7.
4. Switch to .ts, add a build step to strip types.

I don't know how VFileMessage is used; having a consistent runtime shape
by virtue of using class property declarations or initialising all
properties to undefined generally means: faster performance, but more
memory usage. I suspect it doesn't matter because people aren't creating
millions of these objects per second, but I don't really know.

Because TS7 is quite a way off, I don't know whether you'll want to take
this PR. I created it to see how hard it would be to update popular JS
code that uses TS for checking.
@github-actions github-actions bot added the 👋 phase/new Post is being triaged automatically label Jul 23, 2025
@github-actions

This comment has been minimized.

@github-actions github-actions bot added 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Jul 23, 2025
@ChristianMurphy ChristianMurphy requested a review from wooorm July 23, 2025 19:06
@wooorm
Copy link
Member

wooorm commented Jul 23, 2025

Hi there! Thanks for working on the ecosystem!

Using JS instead of TS is intentional for me. Method 3, of using = undefined everywhere, seems the simplest to me then, and most in line with current behavior?

These things are just to document the fields: they are indeed never set by us, but ecosystem users may set and use them.
So, perhaps they could be set in VFileMessage.prototype... at the bottom instead?
Perhaps all fields currently set and documented inside the constructor can go there? Not sure if I tried that before for the ones that we do set?

and the code has some clever manipulation of the prototype to avoid setting file on every object

@sandersn, what’s the clever code here you mention?

@sandersn
Copy link
Contributor Author

I figured that adding a tsc build was not something you wanted. The types.d.ts file I added is effectively a shim to inject types for Typescript to work with without changing anything at runtime. But I am stuck with .d.ts to do this because there's no equivalent jsdoc that Typescript understands.

The clever code I noticed is the VFileMessage.prototype.file = '', etc. It only requires one copy of the properties to provide a default if they're not set. Which is slower than a uniform class shape but saves memory. When I added this.file = undefined, I noticed a test failing, which is how I noticed that code.

If there aren't millions of these objects being created I think I'd go with the = undefined strategy. It wastes a little space for readability. Want me to revert back to the first commit?

@remcohaszing
Copy link
Member

I like option 2 or 3. This is how most people would normally write a class. This class isn’t that special I think.

I never really understood the purpose of the VFileMessage.prototype assignments below the class definition and wouldn’t mind them being removed.

@sandersn
Copy link
Contributor Author

I reverted to the simple change with initialisers in the constructor. To make tests pass, I changed the initial value of file from undefined to '' to match the VFileMessage.prototype assignment at the bottom. I didn't remove those but I can if you want.

@wooorm wooorm merged commit 56ceddd into vfile:main Jul 26, 2025
4 checks passed
@github-actions

This comment has been minimized.

@wooorm wooorm added ☂️ area/types This affects typings 💪 phase/solved Post is done 🏡 area/internal This affects the hidden internals labels Jul 26, 2025
@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Jul 26, 2025
@wooorm
Copy link
Member

wooorm commented Jul 26, 2025

Thanks, released in 4.0.3!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🏡 area/internal This affects the hidden internals ☂️ area/types This affects typings 💪 phase/solved Post is done

Development

Successfully merging this pull request may close these issues.

3 participants