Rename location field to position

[wooorm]

The position field is used throughout unified, only vfile-message uses location. It makes more sense to use the same name everywhere.

[wooorm]

Released in 3.0.0!

[viceice]

Is there any option to add this patch to a v2 bugfix release?

diff --git a/node_modules/vfile-message/types/index.d.ts b/node_modules/vfile-message/types/index.d.ts
index d217843..31c853a 100644
--- a/node_modules/vfile-message/types/index.d.ts
+++ b/node_modules/vfile-message/types/index.d.ts
@@ -6,7 +6,7 @@ declare namespace vfileMessage {
   /**
    * Create a virtual message.
    */
-  interface VFileMessage extends Error {
+  interface VFileMessage extends Omit<Error, 'location'> {
     /**
      * Constructor of a message for `reason` at `position` from `origin`.
      * When an error is passed in as `reason`, copies the `stack`.

I currently use patch-package to fix the compile error here: renovatebot/renovate#9954

[viceice]

Error:

node_modules/vfile-message/types/index.d.ts:9:13 - error TS2430: Interface 'VFileMessage' incorrectly extends interface 'Error'.
  Types of property 'location' are incompatible.
    Type 'Position' is not assignable to type 'string'.

9   interface VFileMessage extends Error {
              ~~~~~~~~~~~~


Found 1 error.

[wooorm]

/cc @ChristianMurphy

[ChristianMurphy]

Hey @viceice! 👋
Happy to see renovate adopting remark!

node_modules/vfile-message/types/index.d.ts:9:13 - error TS2430: Interface 'VFileMessage' incorrectly extends interface 'Error'.
 Types of property 'location' are incompatible.
   Type 'Position' is not assignable to type 'string'.

🤔 What TS version and tsconfig is renovate currently using?
vfile-message is currently testing with:

vfile-message/package.json

Line 46 in fbdb618

"typescript": "^4.0.0",

vfile-message/tsconfig.json

Lines 1 to 15 in fbdb618

	{
	"include": ["*.js"],
	"compilerOptions": {
	"target": "ES2020",
	"lib": ["ES2020"],
	"module": "ES2020",
	"moduleResolution": "node",
	"allowJs": true,
	"checkJs": true,
	"declaration": true,
	"emitDeclarationOnly": true,
	"allowSyntheticDefaultImports": true,
	"skipLibCheck": true
	}
	}

and CI currently seems to be happy https://github.com/vfile/vfile-message/actions/runs/813710479 🤔

interface VFileMessage extends Omit<Error, 'location'>

This could be an option, I'd like to figure out why this project's test suite isn't able to see the issue.
If we do narrow the issue down to the typing, some research will be needed on how to do this with TS based off JSDoc which is how the typings are generated.

vfile-message/index.js

Line 9 in fbdb618

export class VFileMessage extends Error {

---

aside: if you'd be interested in contributing https://github.com/renovatebot/renovate/blob/23799d627c6d2ed570ed92a2d90c4116b0f6d351/lib/types/remark-github.d.ts
remark happily accepts d.ts files as well, along side some dtslint tests, for example remarkjs/strip-markdown#21, remarkjs/remark-external-links#18, and remarkjs/remark-footnotes#4

[viceice]

Sure, happy to fix it soon. I stumbled on that while trying to fix a security issue in linkify-markdown which is using remark but sadly not updated a log time , see renovatebot/renovate#9954.

I think we are on latest stable typescript, currently using v4.2.4. i don't like to enable skipLibCheck because of this small issue. That's why I use patch-package to fix the type for us.

Also we can't move to new esm only version now because some internal projects are not yet ready for it. We plan to transition to esm only hopefully until the end of the year. But maybe later or sooner. 🙃

Maybe a better solution is to provide our types to remark-github so we don't have to care about the type. 😅 Can supply a pr if it's merged soon. 😏

[ChristianMurphy]

Ah, I missed:

v2 bugfix

I was looking at v3.

Switching to v2, same question applies 😅

It is/was being tested with dtslint for typescript versions 3.0+
with configuration

vfile-message/types/tsconfig.json

Lines 1 to 10 in b95250d

	{
	"compilerOptions": {
	"lib": ["es2015"],
	"strict": true,
	"baseUrl": ".",
	"paths": {
	"vfile-message": ["index.d.ts"]
	}
	}
	}

And CI seemed happy: https://travis-ci.org/github/vfile/vfile-message/builds/668455498
I believe 4.0 was included in the test suite, as IIRC it was in the next channel at the time, but am not 💯 sure.

---

That's why I use patch-package to fix the type for us

That may be a good option, in general vfile/unified maintains a single release line at a time, currently v3.

We plan to transition to esm only hopefully until the end of the year

Very cool

Maybe a better solution is to provide our types to remark-github so we don't have to care about the type

Can supply a pr if it's merged soon

When dtslint and a type test file are included in the PR, turn around is usually quick.
It used to take a little longer because types were released as major, out of an abundance of caution, but now IIRC @wooorm generally releases them as minor versions, which speeds up the process (no queuing up other breaking changes to release all at once).

[viceice]

Maybe it's because we are targeting es2018 with node v14 types. So typescript maybe the newer types are causing the issue.

I'm happen to provide the types to the package with other stuff too. Will try tomorrow.

But maybe it would also helpful to have a v2 fix which can bubble through via allowed in range updates. 🙃 I can prepare a pr too for this, hopefully passing your ci, if you will accept that work. Don't want to do unnecessary work. 😉

[ChristianMurphy]

Maybe it's because we are targeting es2018 with node v14 types. So typescript maybe the newer types are causing the issue

🤔 could be

I'm happen to provide the types to the package with other stuff too

Thanks!

But maybe it would also helpful to have a v2 fix which can bubble through via allowed in range updates. I can prepare a pr too for this, hopefully passing your ci, if you will accept that work. Don't want to do unnecessary work.

Thanks for the offer!
I'd defer to @vfile/releasers on that.
I can certainly review it, but the releasers would be needed to get it out to npm.

[viceice]

You also haven't a v2 branch to target a pr to.😉

I'll try to arrange some pr's tomorrow. 🤗

[viceice]

I've made a commit to fix it, npm test works fine, but i can't open a pr, because there is no branch to target v2

https://github.com/viceice/vfile-message/commit/da733c2cc9e4e19a55e8f8a7379e4d4609aaf3b5

[wooorm]

We’ve had these types for 3 years and millions of people have used this. I’m not sure why this is an issue now?

Why not turn on skipLibChecks?

[viceice]

The issue occures is you have newer typescript 4.2 and es2018 types, so maybe others do not yet upgraded? or they enabled skipLibChecks instead of open an issue.

We don't like skipLibChecks, as it can cause hidden type issues for dependends of our lib 😕

[wooorm]

Set up a legacy branch.
Let’s try it out.
I’m not a huge fan of multiple release lines though