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.

Sign up for GitHub

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

Use broader template-lint range #418

Merged
merged 13 commits into from
Nov 27, 2024
Merged

Use broader template-lint range #418

Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
035923e
Update template-linter.ts
NullVoxPopuli Nov 22, 2024
514ae2f
Do a proper comparison
NullVoxPopuli Nov 22, 2024
1859807
Add comment explaining semver.parse
NullVoxPopuli Nov 22, 2024
2dd183c
Don't clean at all -- turns out template-lint is the only thing that …
NullVoxPopuli Nov 22, 2024
827faef
Remove unused import
lifeart Nov 25, 2024
10a09db
import type
lifeart Nov 25, 2024
e2804b1
add failing test
lifeart Nov 25, 2024
5e964ca
+
lifeart Nov 25, 2024
372197b
+
lifeart Nov 25, 2024
95e33c8
use dependenciesMeta for template-lint version
lifeart Nov 25, 2024
8b30ec5
do not lint if no meta for template-lint
lifeart Nov 25, 2024
e44d324
remove unused logic
lifeart Nov 25, 2024
45158a0
update tests
lifeart Nov 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions src/template-linter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Copy link
Contributor

Choose a reason for hiding this comment

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

nope, it's extra bundle size

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@lifeart lifeart Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link

@arthur5005 arthur5005 Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@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

Copy link

@arthur5005 arthur5005 Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@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.
Screenshot 2024-11-22 at 8 50 24 AM

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

^- @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. :\


type FindUp = (name: string, opts: { cwd: string; type: string }) => Promise<string | undefined>;
type LinterVerifyArgs = { source: string; moduleId: string; filePath: string };
Expand Down Expand Up @@ -88,7 +89,7 @@ export default class TemplateLinter {
return this.server.projectRoots.projectForUri(textDocument.uri);
}

private sourcesForDocument(textDocument: TextDocument, templateLintVersion: string): string[] {
private sourcesForDocument(textDocument: TextDocument, templateLintVersion: SemVer | null): string[] {
const ext = getExtension(textDocument);

if (ext !== null && !extensionsToLint.includes(ext)) {
Expand All @@ -98,8 +99,11 @@ export default class TemplateLinter {
const documentContent = textDocument.getText();

// we assume that ember-template-lint v5 could handle js/ts/gts/gjs files
if (!templateLintVersion) {
return [documentContent];
}

if (templateLintVersion >= '5') {
if (semver.gte(templateLintVersion, '5.0.0')) {
return [documentContent];
}
Comment on lines +107 to 109
Copy link

@arthur5005 arthur5005 Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

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


Expand Down Expand Up @@ -152,11 +156,12 @@ export default class TemplateLinter {
}

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

let sources = [];

try {
const linterVersion = linterMeta?.version ? semver.parse(linterMeta.version) : null;
lifeart marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link

@arthur5005 arthur5005 Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

how so? I was just gonna fix the getDepIfExists code

Copy link

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

this approach not going to work for us:
image

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

Copy link

Choose a reason for hiding this comment

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

@lifeart this approach does work, you just need to use minVersion, not parse
Screenshot 2024-11-25 at 11 23 20 AM

Copy link

Choose a reason for hiding this comment

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

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


sources = this.sourcesForDocument(textDocument, linterVersion);
} catch (e) {
return;
Expand Down
Loading