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.

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

Fix bezier-vscode extension bug #2507

Merged
merged 27 commits into from
Dec 18, 2024

Conversation

yangwooseong
Copy link
Collaborator

@yangwooseong yangwooseong commented Nov 25, 2024

Self Checklist

  • I wrote a PR title in English and added an appropriate label to the PR.
  • I wrote the commit message in English and to follow the Conventional Commits specification.
  • I added the changeset about the changes that needed to be released. (or didn't have to)
  • I wrote or updated documentation related to the changes. (or didn't have to)
  • I wrote or updated tests related to the changes. (or didn't have to)
  • I tested the changes in various browsers. (or didn't have to)
    • Windows: Chrome, Edge, (Optional) Firefox
    • macOS: Chrome, Edge, Safari, (Optional) Firefox

Related Issue

  • None

Summary

  • vscode extension 을 실행시키면 아래의 에러가 뜨면서 동작하지 않는 버그를 수정합니다.
Activating extension 'ChannelCorp.bezier-vscode' failed: Cannot find module 'vscode-languageclient/node'
Require stack:
- /Users/mb-188/.cursor/extensions/channelcorp.bezier-vscode-0.3.0/dist/client.js

Details

  • 익스텐션 패키지에 의존성이 담기지 않아서 생기는 버그로 추정됩니다. 왜 0.1.0 버전에서 잘 동작했는지는 잘 모르겠습니다.
  • 의존성을 패키지에 포함시키려면 .vscodeignore 에서 상위 디렉토리에 있는 node_modules 하위의 vscode-* 의존성을 모두 화이트리스트로 지정해야 합니다. 그런데 이렇게 하면 vsce package 명령어에서 에러가 뜹니다.
  • vsce package --no-yarn -> npm 으로 패키지를 시도하면 invalid relative path: extension/../../vscode-languageclient/node 에러가 뜹니다. 상대경로를 제대로 resolve 하지 못하는 것 같습니다.
  • vsce package -> yarn 으로 패키지를 시도하면 vsce 내에서 yarn list --no-prod --no-json 명령어를 수행하는데, 이 명령어는 yarn v1 에서만 있는 명령어라 현재 베지어 레포의 yarn 버전과 맞지 않아서 명령어를 실행할 수 없다는 에러가 뜹니다. yarn list --no-prod --no-json를 yarn 플러그인으로 대체하는 옵션까지 했지만 vsce ls 결과물에 의존성이 포함되지 않았습니다.
  • 그리하여 이 PR 내용처럼 lsp-sample 처럼 client, server 폴더 구조를 분리하고 각 폴더에 의존성을 설치했는데, 이렇게 하면 루트에서 빌드할 때마다 client, server 각각에 의존성을 설치해야 합니다. (#) 아예 bezier-vscode 의 yarn build 스크립트를 실행하면 client, server 의존성 설치를 하도록 해야 할까 싶기도 하네요.. 좋은 생각 있으신가요? @sungik-choi

정리
vsce package/publish 에서 상위 디렉토리에 있는 의존성을 가져올 수 없는 문제때문에 server, client 패키지를 beizer-vscode 패키지 안에 만들고, package/publish 단계에서 server,client 패키지안에 의존성을 설치

  • 의존성을 패키지에 포함하면 위의 에러가 안뜨는 것은 확인했습니다. 로컬에서 패키징해서 설치하려면 client, server 의존성 설치 후 루트에서 yarn workspace bezier-vscode package 로 나오는 .vsix 파일을 우클릭해서 설치 하면 됩니다.

Breaking change? (Yes/No)

  • No

References

  • None

@yangwooseong yangwooseong added fix PR related to bug fix bezier-vscode Issue or PR related to bezier-vscode labels Nov 25, 2024
@yangwooseong yangwooseong self-assigned this Nov 25, 2024
Copy link

channeltalk bot commented Nov 25, 2024

Copy link

changeset-bot bot commented Nov 25, 2024

🦋 Changeset detected

Latest commit: 9d9c1a9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
bezier-vscode Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@yangwooseong yangwooseong marked this pull request as draft November 25, 2024 06:06
Copy link

codecov bot commented Nov 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.88%. Comparing base (d89ac74) to head (9d9c1a9).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2507   +/-   ##
=======================================
  Coverage   81.88%   81.88%           
=======================================
  Files         145      145           
  Lines        2887     2887           
  Branches      918      918           
=======================================
  Hits         2364     2364           
  Misses        493      493           
  Partials       30       30           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yangwooseong yangwooseong force-pushed the WEB-5626/bezier-vscode branch 2 times, most recently from 6cfb1cb to e468d98 Compare December 10, 2024 09:04
@yangwooseong yangwooseong marked this pull request as ready for review December 10, 2024 09:07
Copy link
Contributor

Choose a reason for hiding this comment

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

bezier-vscode, client, server의 yarn.lock 파일은 버전관리하지 않아도 될 거 같아요

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

버전관리 하지 않는 다는 의미를 이해못했습니다..!

Copy link
Contributor

Choose a reason for hiding this comment

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

git에 포함할 필요가 없을 거 같다는 뜻이었어요

Copy link
Contributor

@sungik-choi sungik-choi Dec 11, 2024

Choose a reason for hiding this comment

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

image
  "workspaces": [
    "client",
    "server"
  ],
  "installConfig": {
    "hoistingLimits": "dependencies"
  },

테스트하다 발견한건데, workspace와 installConfig.hoistingLimits를 혼합해서 사용하면 cd - install 없이 루트에서 node_modules 생성 위치를 제어할 수 있네요. 이 방법으로 테스트해봐주실 수 있나요?

이 방식을 적용하면 root workspace에 추가한 "packages/bezier-vscode/*" path도 제거할 수 있을 거 같아요

image

yarn install 결과

Copy link
Contributor

Choose a reason for hiding this comment

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

이 방식을 적용하면 root workspace에 추가한 "packages/bezier-vscode/*" path도 제거할 수 있을 거 같아요

제거할경우 터보레포의 태스크 의존성 관리가 안되어서, 아래 태스크를 추가해야하네요

    "bezier-vscode#build": {
      "outputs": ["dist/**"], // client, server도 추가?
      "dependsOn": ["@channel.io/bezier-tokens#build"]
    },

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

이거 이렇게 하고 패키징 해보니까 language server client connection 이 안된다는 에러가 뜨네요 😢

image

Copy link
Contributor

Choose a reason for hiding this comment

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

설치되는 의존성은 동일할 거 같은데...

Copy link
Collaborator Author

@yangwooseong yangwooseong Dec 13, 2024

Choose a reason for hiding this comment

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

이전에는 client/node_modules 밑에 client 가 필요한 의존성을 다 들고 있었는데, 지금은 소수만(vscode-languageclient)들고 있고 대부분은 호이스팅되서 bezier-vscode/node_moduls로 빠지네요. 아예 다 호이스팅해버리거나 안하거나 하면 괜찮아질 것 같아서 그 방법을 찾아봐야 할 것 같습니다. -> client, server 각각의 package.json 에서 hoistingLimits 를 설정하니 되는것 같네요. 이걸로 다시 해볼게요

image

@@ -60,16 +60,8 @@
"typecheck": "tsc --noEmit",
"clean": "rm -rf dist",
Copy link
Contributor

Choose a reason for hiding this comment

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

client, server의 dist와 node_modules 등도 지웠으면 좋겠습니다

turbo.json Outdated
Comment on lines 23 to 26
"bezier-vscode#build": {
"outputs": ["client/dist/**", "server/dist/**"],
"dependsOn": ["@channel.io/bezier-tokens#build"]
},
Copy link
Contributor

Choose a reason for hiding this comment

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

제거해도 될 거 같아요

@yangwooseong yangwooseong force-pushed the WEB-5626/bezier-vscode branch from 7431421 to da22761 Compare December 18, 2024 01:23
@yangwooseong
Copy link
Collaborator Author

  • a453056 부터 방향을 다시 바꿔서, node_modules 를 패키지 결과에 그대로 포함하지 않고 의존성까지 번들링한 파일(client.js, server.js)만 패키지에 포함하는 것으로 작업했습니다. 이렇게 하면 의존성을 신경 쓸 필요가 없어져서 hoistingLimits 을 제거해도 되고, vsce package에 --no-dependencies 옵션만 주면 패키징이 잘 됩니다.
  • 설치하고 런타임 에러 났던 것은 client.ts에서 잘못된 경로를 보고 있어서 그랬던 것 같아요.
  • vscode 1.96 버전이 가장 최신 버전이라 낮은 버전에서는 extension 설치가 안되서 engines.vscode 버전을 낮췄습니다.

Copy link
Contributor

@sungik-choi sungik-choi left a comment

Choose a reason for hiding this comment

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

👍 이 편이 훨씬 좋네요

@yangwooseong yangwooseong force-pushed the WEB-5626/bezier-vscode branch from 8c2d76c to 9d9c1a9 Compare December 18, 2024 04:22
@yangwooseong yangwooseong merged commit bc0c694 into channel-io:main Dec 18, 2024
3 checks passed
sungik-choi pushed a commit that referenced this pull request Dec 19, 2024
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## @channel.io/[email protected]

### Minor Changes

- Add SCSS support to access design tokens directly through SCSS
variables.
([#2568](#2568)) by
@sungik-choi

## @channel.io/[email protected]

### Patch Changes

- Export the `alphaTokens`, `AlphaTokens`, and `useAlphaTokens` modules.
([#2564](#2564)) by
@sungik-choi

-   Updated dependencies
    -   @channel.io/[email protected]

## @channel.io/[email protected]

### Patch Changes

-   Updated dependencies
    -   @channel.io/[email protected]

## [email protected]

### Minor Changes

- Fixed bug where autocomplete did not work.
([#2507](#2507)) by
@yangwooseong

### Patch Changes

-   Updated dependencies
    -   @channel.io/[email protected]

## [email protected]

### Patch Changes

-   Updated dependencies
    -   @channel.io/[email protected]

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bezier-vscode Issue or PR related to bezier-vscode fix PR related to bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants