Use broader template-lint range #418

NullVoxPopuli · 2024-11-22T15:18:21Z

Closes #417

arthur5005 · 2024-11-22T15:20:00Z

src/template-linter.ts

-    if (templateLintVersion === '5') {
+    if (templateLintVersion >= '5') {
      return [documentContent];
    }


@NullVoxPopuli this isn't enough I don't believe, the templateLintVersion might come in as a ^ or a ~

excellent point -- I'll update this to use semver

@NullVoxPopuli we need parseInt here :)

not if it's a ^ -- gonna use the semver package. sorry for the low effort initial PR!

@NullVoxPopuli it's not a case - we have this precondition - https://github.com/ember-tooling/ember-language-server/blob/master/src/utils/layout-helpers.ts#L293

lifeart

seems we need to convert string to int

lifeart · 2024-11-22T15:54:47Z

src/template-linter.ts

-    if (templateLintVersion === '5') {
+    if (templateLintVersion >= '5') {
      return [documentContent];
    }


@NullVoxPopuli we need parseInt here :)

lifeart · 2024-11-22T16:00:53Z

src/template-linter.ts

@@ -12,6 +12,7 @@ import Server from './server';
 import { Project } from './project';
 import { getRequireSupport } from './utils/layout-helpers';
 import { getFileRanges, RangeWalker } from './utils/glimmer-script';
+import semver, { SemVer } from 'semver';


nope, it's extra bundle size

Ah, my bad, we already have it in layout helpers, we good in this case.

but it seems we don't need it anyway for our case

@lifeart why not? The raw string ^5.13.0 for example can't be read as versionString[0] otherwise you get a version ^ no?

@arthur5005 it's not a case, we already cleaning value here: https://github.com/ember-tooling/ember-language-server/blob/master/src/utils/layout-helpers.ts#L293

@lifeart just tested how clean works, I think it doesn't work how we think it's supposed to work, unless I'm missing something.

I might not know how the package strings pre-processed though before reaching the clean method.

@arthur5005 oh my, thank you for pointing to it!

^- @NullVoxPopuli To @lifeart's point, I think how we parse dependency versions is the real problem here, I'm imagining the version on the dep meta is null for all unpinned projects. :\

src/template-linter.ts

lifeart · 2024-11-22T17:21:53Z

src/template-linter.ts

+       *
+       * (same as when errors are thrown from sourcesForDocument)
+       */
+      const linterVersion = linterMeta?.version ? semver.parse(linterMeta.version) : null;


it seems linterMeta.version may be already "broken" by semver.clean
#418 (comment)

Seems we need to add some tests to ensure how it's actually works for real-life cases.

Best to just set the version directly, and expect all users to have to use semver to interact with the value. Unless there's a unified way to get the locked / installed version.

@arthur5005 another option I see here - is to get actual version from installed ember-template-lint package. In this case we could get just exact value, without prefixes and postfixes.

cc @NullVoxPopuli seems code landed in https://github.com/ember-tooling/ember-language-server/pull/411/files#diff-6162d8c9873071bf9f26293887dc9669d8c375d34f5e3457fc57e2cbc17cbb60R41 may help here..

@lifeart yeah, if there's a reliable way to get exact versions across all package managers, let's do that.

how so? I was just gonna fix the getDepIfExists code

I think that's fine too. I think @lifeart would prefer getting exact versions, but we could probably live with major version only checking for now.

We just need to fix that ts error All imports in import declaration are unused. and I think this is ready @NullVoxPopuli

…uses this data

lifeart · 2024-11-25T18:48:49Z

src/template-linter.ts

+       *
+       * (same as when errors are thrown from sourcesForDocument)
+       */
+      const linterVersion = linterMeta?.version ? semver.parse(linterMeta.version) : null;


this approach not going to work for us:

I agree with @arthur5005 - we need to implement proper version resolution from Project.dependencyMap

@lifeart this approach does work, you just need to use minVersion, not parse

The other approach is just use the method to get the SemVer object directly, and put that in the meta instead of the string.

lifeart · 2024-11-25T19:08:24Z

src/template-linter.ts

@@ -151,12 +156,24 @@ export default class TemplateLinter {
      return;
    }

-    const linterMeta = project.dependenciesMeta.find((dep) => dep.name === 'ember-template-lint');
-    const linterVersion = linterMeta?.version.split('.')[0] || 'unknown';
+    const linterMeta = project.dependencyMap.get('ember-template-lint');


likely we need to add integration test for it, or test locally :)

arthur5005 · 2024-11-25T19:33:51Z

src/template-linter.ts

+    if (semver.gte(templateLintVersion, '5.0.0')) {
      return [documentContent];
    }


This doesn't work sadly.
Something like the following is technically correct:

semver.gte(semver.minVersion(templateLintVersion)?.version ?? '0.0.0', '5.0.0')

arthur5005 · 2024-11-25T19:36:34Z

src/template-linter.ts

+       * (same as when errors are thrown from sourcesForDocument)
+       */
+      const version = linterMeta?.package.version;
+      const linterVersion = version ? semver.parse(version) : null;


semver.minVersion(version)?.version ?? '0.0.0'

I updated logic on how we resolve template-lint version, now it should be taken from deps, need to test it...

NullVoxPopuli · 2024-11-25T21:18:47Z

it works

Update template-linter.ts

035923e

NullVoxPopuli marked this pull request as ready for review November 22, 2024 15:18

arthur5005 suggested changes Nov 22, 2024

View reviewed changes

lifeart requested changes Nov 22, 2024

View reviewed changes

Do a proper comparison

514ae2f

lifeart reviewed Nov 22, 2024

View reviewed changes

src/template-linter.ts Outdated Show resolved Hide resolved

Add comment explaining semver.parse

1859807

lifeart reviewed Nov 22, 2024

View reviewed changes

NullVoxPopuli and others added 3 commits November 22, 2024 18:36

Don't clean at all -- turns out template-lint is the only thing that …

2dd183c

…uses this data

Remove unused import

827faef

import type

10a09db

lifeart approved these changes Nov 25, 2024

View reviewed changes

add failing test

e2804b1

lifeart reviewed Nov 25, 2024

View reviewed changes

lifeart added 4 commits November 25, 2024 21:52

+

5e964ca

+

372197b

use dependenciesMeta for template-lint version

95e33c8

do not lint if no meta for template-lint

8b30ec5

lifeart reviewed Nov 25, 2024

View reviewed changes

remove unused logic

e44d324

arthur5005 reviewed Nov 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use broader template-lint range #418

Use broader template-lint range #418

NullVoxPopuli commented Nov 22, 2024 •

edited

Loading

arthur5005 Nov 22, 2024

NullVoxPopuli Nov 22, 2024

lifeart Nov 22, 2024

NullVoxPopuli Nov 22, 2024

lifeart Nov 22, 2024

lifeart left a comment

lifeart Nov 22, 2024

lifeart Nov 22, 2024

lifeart Nov 22, 2024

lifeart Nov 22, 2024 •

edited

Loading

arthur5005 Nov 22, 2024 •

edited

Loading

lifeart Nov 22, 2024

arthur5005 Nov 22, 2024 •

edited

Loading

lifeart Nov 22, 2024

arthur5005 Nov 22, 2024

lifeart Nov 22, 2024

arthur5005 Nov 22, 2024

lifeart Nov 22, 2024

lifeart Nov 22, 2024

arthur5005 Nov 22, 2024 •

edited

Loading

NullVoxPopuli Nov 22, 2024

arthur5005 Nov 22, 2024

arthur5005 Nov 25, 2024

lifeart Nov 25, 2024

arthur5005 Nov 25, 2024

arthur5005 Nov 25, 2024

lifeart Nov 25, 2024

arthur5005 Nov 25, 2024 •

edited

Loading

arthur5005 Nov 25, 2024

lifeart Nov 25, 2024

NullVoxPopuli commented Nov 25, 2024

Use broader template-lint range #418

Are you sure you want to change the base?

Use broader template-lint range #418

Conversation

NullVoxPopuli commented Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lifeart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lifeart Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

arthur5005 Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arthur5005 Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arthur5005 Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arthur5005 Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullVoxPopuli commented Nov 25, 2024

NullVoxPopuli commented Nov 22, 2024 •

edited

Loading

lifeart Nov 22, 2024 •

edited

Loading

arthur5005 Nov 22, 2024 •

edited

Loading

arthur5005 Nov 22, 2024 •

edited

Loading

arthur5005 Nov 22, 2024 •

edited

Loading

arthur5005 Nov 25, 2024 •

edited

Loading