Skip to content

Conversation

@datho7561
Copy link
Contributor

@datho7561 datho7561 commented Jul 15, 2025

What does this PR do?

Change the relative fallback path of the l10n bundle so that it works even if the language client doesn't send the path in the initialization options.

What issues does this PR fix or reference?

If the language server client doesn't specify the location of the l10n folder, then the language server falls back to a path relative to the complied file. This path was wrong; server.js is in ./out/server/src/server.js, so the relative path of the l10n folder is ../../../l10n instead of ../l10n.

Is it tested? How?

  1. Open vscode-yaml and comment out https://github.com/redhat-developer/vscode-yaml/blob/main/src/extension.ts#L134 so the language client doesn't specify where the l10n path is.
  2. Run the extension and verify that the language features still work.

@datho7561
Copy link
Contributor Author

datho7561 commented Jul 15, 2025

I need to fix the tests done

@datho7561 datho7561 force-pushed the fix-l10n-fallback-path branch from b775865 to 73ddd22 Compare July 15, 2025 14:15
@coveralls
Copy link

coveralls commented Jul 15, 2025

Coverage Status

coverage: 83.956% (-0.02%) from 83.971%
when pulling 01ca741 on datho7561:fix-l10n-fallback-path
into 5215810 on redhat-developer:main.

If the language server client doesn't specify the location of the l10n
folder, then the language server falls back to a path relative to the
complied file.
This path was wrong; `server.js` is in `./out/server/src/server.js`,
so the relative path of the `l10n` folder is `../../../l10n` instead of
`../l10n`.

Signed-off-by: David Thompson <[email protected]>
@datho7561 datho7561 force-pushed the fix-l10n-fallback-path branch from 73ddd22 to 01ca741 Compare July 15, 2025 14:21
Copy link
Contributor

@msivasubramaniaan msivasubramaniaan left a comment

Choose a reason for hiding this comment

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

LGTM

@msivasubramaniaan msivasubramaniaan merged commit bf09a39 into redhat-developer:main Jul 15, 2025
4 checks passed
@datho7561 datho7561 deleted the fix-l10n-fallback-path branch July 15, 2025 14:33
@lotheac
Copy link
Contributor

lotheac commented Jul 15, 2025

@datho7561 as a distro packager (with very little js-specific experience): I find it a little surprising that the build now produces an out/ directory, but its contents depend on files outside of that out/ directory (ignoring library dependencies).

At least the Alpine package (which I maintain) and the Arch PKGBUILD are manually copying the l10n json directory into the distro package, and it's a bit of an extra step for packagers to figure out that it was wrong to assume that the build output directory contains everything necessary.

I wonder if you would consider copying the l10n json files to a location in the out/ directory instead?

(and sorry for being so slow with this comment that this was merged in the meantime :)

msivasubramaniaan pushed a commit to msivasubramaniaan/yaml-language-server that referenced this pull request Jul 15, 2025
If the language server client doesn't specify the location of the l10n
folder, then the language server falls back to a path relative to the
complied file.
This path was wrong; `server.js` is in `./out/server/src/server.js`,
so the relative path of the `l10n` folder is `../../../l10n` instead of
`../l10n`.

Signed-off-by: David Thompson <[email protected]>
msivasubramaniaan pushed a commit to msivasubramaniaan/yaml-language-server that referenced this pull request Jul 15, 2025
If the language server client doesn't specify the location of the l10n
folder, then the language server falls back to a path relative to the
complied file.
This path was wrong; `server.js` is in `./out/server/src/server.js`,
so the relative path of the `l10n` folder is `../../../l10n` instead of
`../l10n`.

Signed-off-by: David Thompson <[email protected]>
msivasubramaniaan pushed a commit to msivasubramaniaan/yaml-language-server that referenced this pull request Jul 15, 2025
If the language server client doesn't specify the location of the l10n
folder, then the language server falls back to a path relative to the
complied file.
This path was wrong; `server.js` is in `./out/server/src/server.js`,
so the relative path of the `l10n` folder is `../../../l10n` instead of
`../l10n`.

Signed-off-by: David Thompson <[email protected]>
msivasubramaniaan pushed a commit to msivasubramaniaan/yaml-language-server that referenced this pull request Jul 15, 2025
If the language server client doesn't specify the location of the l10n
folder, then the language server falls back to a path relative to the
complied file.
This path was wrong; `server.js` is in `./out/server/src/server.js`,
so the relative path of the `l10n` folder is `../../../l10n` instead of
`../l10n`.

Signed-off-by: David Thompson <[email protected]>
@datho7561
Copy link
Contributor Author

If we copy the l10n into out, then there will be two copies of l10n in the published npm package, one in ./l10n and one in ./out/l10n.

JS projects have very freeform build processes, unlike Maven projects or cargo projects. You'll need to understand the build process in order to repackage this package correctly, especially if you are adding other steps like bundling the code into one file.

msivasubramaniaan pushed a commit to msivasubramaniaan/yaml-language-server that referenced this pull request Jul 15, 2025
If the language server client doesn't specify the location of the l10n
folder, then the language server falls back to a path relative to the
complied file.
This path was wrong; `server.js` is in `./out/server/src/server.js`,
so the relative path of the `l10n` folder is `../../../l10n` instead of
`../l10n`.

Signed-off-by: David Thompson <[email protected]>
msivasubramaniaan added a commit that referenced this pull request Jul 15, 2025
If the language server client doesn't specify the location of the l10n
folder, then the language server falls back to a path relative to the
complied file.
This path was wrong; `server.js` is in `./out/server/src/server.js`,
so the relative path of the `l10n` folder is `../../../l10n` instead of
`../l10n`.

Signed-off-by: David Thompson <[email protected]>
Co-authored-by: David Thompson <[email protected]>
@p-spacek
Copy link
Contributor

Hey @datho7561, I am also a little bit worried about this change
the 'static' path '../../../l10n' works ok for local build when you have 'yaml-language-server' located next to this 'vscode-yaml' project in filesystem. https://github.com/redhat-developer/vscode-yaml/blob/4b1730412b5a6bf82f0bc23f6665db8904bf4c21/src/extension.ts#L116

if you build the project with webpack and link the package, the l10n folder is missing
if you compile YLS by

  • 'yarn compile', the structure is also a little bit different (out/server/src/yamlServerInit)
  • 'yarn compile:umd', the structure is also a little bit different (lib/esm/yamlServerInit)

So note that,

  • if you clone vscode-yaml repo from scratch and try to build it, the extension will fail, because '../yaml-language-server/l10n' is not in your filesystem automatically
  • if you build extension with webpack, the fallback path refers outside of the extension to ''/Users/john/.vscode/l10n/bundle.l10n.json''

@datho7561
Copy link
Contributor Author

Yeah, that's not good. The fallback path is pretty useless except for when developing the extension with yaml-language-server cloned and in the same folder. The correct path needs to be passed in from the client during initialization otherwise nothing works as expected.

I can't get vscode-yaml to launch if yaml-language-server isn't in the same folder anyways.

What actions can we take to address this? I guess making the directory structures for UMD, ESM, and whatever the default build is the same would help.

@datho7561
Copy link
Contributor Author

Also why TF is tsconfig.json marked as executable?

@datho7561
Copy link
Contributor Author

Some history on the directory structure: #305

I want to move the cjs code (the output of npm run compile) to under ./lib/cjs (eg. ./lib/cjs/server.js) however this would mean getting rid of ./out/server/src/, which apparently might break some clients. (It'll break vscode-yaml but we can fix that).

But really, I think most clients launch yaml-language-server through the binary and communicate through stdio. I don't know how many people/projects we will actually break. It will be breaking API, so we might need to call the release 2.0.0?

Also, realistically we don't need both the cjs and umd output, the umd output should be sufficient.

phunguyenmurcul added a commit to macrosreply/yaml-language-server that referenced this pull request Oct 3, 2025
* checked const value for the boolean type

* test case added

* added positive test case as well

* Fix corner case for 'flow sequence forbidden' quickfix

- Do not include any trailing spaces in the error range
  - This is a change in behaviour; it seems this behaviour was
    intentional based on the test cases,
    but I don't agree with it, since the error range should only cover
    the erroneous code.
- Fix a bug where `]` or `}` are not included in the error range if they're the last character in the document

Fixes redhat-developer#1060

Signed-off-by: David Thompson <[email protected]>

* added dockercompose and github actions in setting handler

Signed-off-by: msivasubramaniaan <[email protected]>

* Fix YAML JSON Schema files starting with %YAML 1.x

Fix a bug where YAML files containing a JSON Schema that start with
%YAML 1.x or a comment that contains a number would fail to to be parsed.

This is caused by the fact that the JSON Schema parser would contain the
number, e.g. 1.2 as the value of `unresolvedJsonSchema.schema` and so
the test for `=== undefined` would fail. This would cause the error from
the JSON parser to be returned instead of trying to parse the file as a
YAML file.

To work around this, also check if `unresolvedJsonSchema.schema` is a
number. It if is, it clearly was not a JSON Schema and so we can safely
try to parse the file as a YAML file.

Fixes: redhat-developer#922
Signed-off-by: David Lechner <[email protected]>

* show the matching value on top

* fix eslint issue

* removed enumMarkdown entries

* Fix bool enums (redhat-developer#1080)

* add (failing) test case for boolean enum and const values

failing with:

```
[
  {
    code: 1,
    data: {
      schemaUri: [ 'file:///default_schema_id.yaml' ],
      values: [ true, false ]
    },
    message: 'Value is not accepted. Valid values: true, false.',
    range: {
      end: { character: 15, line: 0 },
      start: { character: 11, line: 0 }
    },
    severity: 1,
    source: 'yaml-schema: file:///default_schema_id.yaml'
  }
]
```

* fix the issue for `boolean` values in `enum`

this is the same fix for `enum` that has been applied a while back for `const` values here: redhat-developer@e6165e4

fixes issue: redhat-developer#1078

---------

Co-authored-by: xxx <xxx>

* Support draft-04 schemas (redhat-developer#1065)

This helps to support the schema for openapi 3.0.0,
among others, that use an older draft of JSON schema that has some type
differences that make it incompatible with JSON schema draft 07.

See redhat-developer#1006 , I think the root cause for the issues that PR caused
is that we were trying to download and cache the metaschema from
the "URL" instead of using the copy that's bundled with `ajv`.

Fixes redhat-developer#780, Fixes redhat-developer#752

(and many, many duplicates we'll have to find and clean up)

Signed-off-by: David Thompson <[email protected]>

* Fix recursion bug in ast-conversion (redhat-developer#1076)

* Ensure recursion works as expected

* fix test safety for when parse fails

* use a cache to keep track of resolved aliases

* Added l10n bundle for various language and translate the local strings (redhat-developer#1086)

* added l10n bundle for various language and translate the local strings

* upstream node

* upstream node

* update to node 18

* Changed NodeJS.Timeout as per node 18

* upstream to node 20

* updated coversall script

* fixed test case

* override json service translation

* fixed test cases

* fix/improve enum value descriptions for merged enum lists (redhat-developer#1085)

This is a follow-up submission for redhat-developer#1028, which removes duplicate enum values in merged enum lists.

This fix/improvement ensures that the first **non-empty** enum description is used for the corresponding enum value.

Before this fix: if the first non-unique enum value was not providing a description but the second one was, it was not
picked up and the enum value description stayed empty.

Co-authored-by: xxx <xxx>
Co-authored-by: Muthurajan Sivasubramanian <[email protected]>

* added test cases for bundle and configuration through testhelper class

* Fix array of const completion

* Fix ignoreScalar in value completion

* apply patch

* Updated changelog for the release 1.19.0 (redhat-developer#1094)

* Updated changelog for the release 1.19.0

* updated chhangelog.md

* addressed review comments

* @vscode/l10n is a runtime dependency (redhat-developer#1096)

* check NODE_AUTH_TOKEN is available or not (redhat-developer#1098)

Signed-off-by: msivasubramaniaan <[email protected]>

* Fix auth on npm publish (redhat-developer#1099)

* check NODE_AUTH_TOKEN is available or not

Signed-off-by: msivasubramaniaan <[email protected]>

* set NODE_AUTH_TOKEN

---------

Signed-off-by: msivasubramaniaan <[email protected]>

* Fix auth on npm publish (redhat-developer#1100)

* check NODE_AUTH_TOKEN is available or not

Signed-off-by: msivasubramaniaan <[email protected]>

* set NODE_AUTH_TOKEN

* set npmAuthToken $NODE_AUTH_TOKEN

* set npmAuthToken $NODE_AUTH_TOKEN

---------

Signed-off-by: msivasubramaniaan <[email protected]>

* Update CI.yaml (redhat-developer#1101)

just set token to 123 and check what error occur

* Revert "Update CI.yaml (redhat-developer#1101)" (redhat-developer#1102)

This reverts commit 8c4c22f.

* Fix auth on npm publish (redhat-developer#1103)

* check NODE_AUTH_TOKEN is available or not

Signed-off-by: msivasubramaniaan <[email protected]>

* set NODE_AUTH_TOKEN

* set npmAuthToken $NODE_AUTH_TOKEN

* set npmAuthToken $NODE_AUTH_TOKEN

* all the required parameters are set

---------

Signed-off-by: msivasubramaniaan <[email protected]>

* Fix the fallback path to the l10n folder (redhat-developer#1104)

If the language server client doesn't specify the location of the l10n
folder, then the language server falls back to a path relative to the
complied file.
This path was wrong; `server.js` is in `./out/server/src/server.js`,
so the relative path of the `l10n` folder is `../../../l10n` instead of
`../l10n`.

Signed-off-by: David Thompson <[email protected]>

* Fix the fallback path to the l10n folder (redhat-developer#1104) (redhat-developer#1105)

If the language server client doesn't specify the location of the l10n
folder, then the language server falls back to a path relative to the
complied file.
This path was wrong; `server.js` is in `./out/server/src/server.js`,
so the relative path of the `l10n` folder is `../../../l10n` instead of
`../l10n`.

Signed-off-by: David Thompson <[email protected]>
Co-authored-by: David Thompson <[email protected]>

* Fix NPM broken (redhat-developer#1095)

* set always-auth true

* added registry url and release ci on main push for testing

* removed main push

* renamed to NPM_TOKEN

* used YARN_NPM_AUTH_TOKEN

* used npm_token

* used YARN_NPM_AUTH_TOKEN

* added registry-url

* tried NODE_AUTH_TOKEN

* created .npmrc file

* removed .npmrc

* check NODE_AUTH_TOKEN available or not

* moved the check code on top

* checked NPM_TOKEN

* tried with NODE_AUTH_TOKEN

* Try with NPM instead of YARN

* switch to npm

* removed install command

* fix build issue

* fix build issue

* done pkg fix

* Completely moved to npm from yarn

* review comments addressed

* Bump serialize-javascript and mocha

Bumps [serialize-javascript](https://github.com/yahoo/serialize-javascript) to 6.0.2 and updates ancestor dependency [mocha](https://github.com/mochajs/mocha). These dependencies need to be updated together.


Updates `serialize-javascript` from 6.0.0 to 6.0.2
- [Release notes](https://github.com/yahoo/serialize-javascript/releases)
- [Commits](yahoo/serialize-javascript@v6.0.0...v6.0.2)

Updates `mocha` from 9.2.2 to 11.7.1
- [Release notes](https://github.com/mochajs/mocha/releases)
- [Changelog](https://github.com/mochajs/mocha/blob/main/CHANGELOG.md)
- [Commits](mochajs/mocha@v9.2.2...v11.7.1)

---
updated-dependencies:
- dependency-name: serialize-javascript
  dependency-version: 6.0.2
  dependency-type: indirect
- dependency-name: mocha
  dependency-version: 11.7.1
  dependency-type: direct:development
...

Signed-off-by: dependabot[bot] <[email protected]>

* Remove coveralls dependency (redhat-developer#1107)

From my understanding, I think we just need the GitHub Action in order
for things to work.

Signed-off-by: David Thompson <[email protected]>

* set .npmrc (redhat-developer#1109)

* add encoding dependency

* upstream @vsode/l10n dependency

* updated changelog.md (redhat-developer#1111)

* Fixed broken link to JSON schema website

* Fix release GitHub Action

- setup-node@v5 creates a .npmrc if you provide a registry,
  so there should be no need to do that separately
- after setup-node@v5 is run, `npm publish` expects the token to be
  accessible from the environment variable `NODE_AUTH_TOKEN`
- also fix coveralls, because that's getting annoying
  (root cause is that it tried to publish the results for each platform
  instead of just once)

See https://github.com/actions/setup-node/blob/main/docs/advanced-usage.md#publish-to-npmjs-and-gpr-with-npm

Signed-off-by: David Thompson <[email protected]>

* Fix prerelease publishing

The options that were being used in `npm publish` to modify the version
to include the git hash at the end are intended to be used
with the `npm version` command beforehand instead.
I think this got messed up somewhere along the way while rewriting the
GitHub Action.

Fixes redhat-developer#1128

Signed-off-by: David Thompson <[email protected]>

* Upversion to 1.19.1

We burned the 1.19.0 release by publishing a prerelease version labelled
1.19.0. In order to release, we'll need to use a different version
number.

Signed-off-by: David Thompson <[email protected]>

* Upversion to 1.20.0

* remove \n in README (redhat-developer#1068)

fix typo

Signed-off-by: roc <[email protected]>
Co-authored-by: David Lechner <[email protected]>

* Wrapping code action title with string (redhat-developer#1117)

* Wrapping code action title with string

* adding textedit value field wrapped as string

* README.md: Mention kate as a Client

The `kate` editor has `yaml-language-server` as its default for the YAML format built-in (no plugins or manual configuration needed).

* Support `yaml-textmate` and `yaml-tmlanguage` languages

* redhat-developer/vscode-yaml#1093

* fix: cast cleanupInterval to any before clearing interval

* fix: remove max-warnings option from lint scripts for better flexibility

* Remove @microsoft/eslint-formatter-sarif from devDependencies in package.json

---------

Signed-off-by: David Thompson <[email protected]>
Signed-off-by: msivasubramaniaan <[email protected]>
Signed-off-by: David Lechner <[email protected]>
Signed-off-by: dependabot[bot] <[email protected]>
Signed-off-by: roc <[email protected]>
Co-authored-by: msivasubramaniaan <[email protected]>
Co-authored-by: David Thompson <[email protected]>
Co-authored-by: David Lechner <[email protected]>
Co-authored-by: Kosta <[email protected]>
Co-authored-by: David Thompson <[email protected]>
Co-authored-by: pjsk-stripe <[email protected]>
Co-authored-by: Muthurajan Sivasubramanian <[email protected]>
Co-authored-by: ShadiestGoat <[email protected]>
Co-authored-by: Daniel M. Capella <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: debbie <[email protected]>
Co-authored-by: roc <[email protected]>
Co-authored-by: David Lechner <[email protected]>
Co-authored-by: Arunvenmany <[email protected]>
Co-authored-by: Niels Thykier <[email protected]>
Co-authored-by: RedCMD <[email protected]>
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.

5 participants