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

feat: Add web extension support for syntax highlighting #2323

Merged

Conversation

mm-broadcom
Copy link
Contributor

Adds support to build a web extension that contains a subset of features from the main Cobol LSP extension. Currently only includes the syntax highlighting.

How Has This Been Tested?

This has been tested through building the extension and checking the functionality thereof within VS Code for Web following this guide. No unit tests were created this only provides the syntaxes developed for the main LSP extension within a web compatible extension.

Checklist:

  • Each of my commits contains one meaningful change
  • I have performed rebase of my branch on top of the development
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
    • Not applicable. See the "How Has This Been Tested?" section above.
  • New and existing unit tests pass locally with my changes
  • I have checked my code and corrected any misspellings

@VitGottwald
Copy link
Contributor

Looks good. Do you think you could use esbuild for the bundling instead of webpack?

Adds support to build a web extension that contains a subset of features from the main Cobol LSP extension. Currently only includes the syntax highlighting.
Limit/Disable the commands available when it is run as a web extension. Add documentation denoting that the extension's capabilities are limited as well.
Updated the web extension's output directory to use the dist/web/ folder instead of the root of the dist one.
Updated the build.sh to build the web extension.
Update the npm build:web command to use the browser as the platform rather than node.
Changed the workspace detection in package.json to use the "isWeb" variable rather than virtual workspaces. (Web is a subset of use cases when the filesystem is virtual.)
Add the package command to create the web compatible extension. Also exclude the extension.js and associated source map from being ignored in vscodeignore.
Replaced webpack with esbuild. Fixed test:web command such that the tests can run.
Add web build step to the build.yml
Removed unused Webpack-related packages.
Add package:web step to Github workflow
Update package-lock.json
Updated package-lock.json
@mm-broadcom mm-broadcom marked this pull request as ready for review June 10, 2024 16:21
Comment on lines 349 to 353
- name: Package COBOL LS vsix for Web
working-directory: clients/cobol-lsp-vscode-extension
run: |
npm run package:web
cp *.vsix ../../.
Copy link
Contributor

Choose a reason for hiding this comment

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

This will run 6x (once in each native configuration) and possibly overwrite the "desktop" VSIX package.
I suggest creating a separate job (e.g. buildWeb) in this file that will just do

  • checkout
  • "update version for PR" step
  • npm ci
  • npm run test:web
  • npm run package:web
  • upload artifact to preserve the VSIX

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It makes a separate vsix file with the name cobol-language-support-web-[version] where version is the latest numbered version. (2.1.2 as of this writing.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made a separate job for building the web extension. Will need some help with the upload as there isn't an existing one to handle the artifact.

Copy link
Contributor

Choose a reason for hiding this comment

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

- uses: actions/upload-artifact@v3
with:
if-no-files-found: warn
name: vsix-cobol-language-support-${{ env.target }}
path: 'cobol-language-support*.vsix'

clients/cobol-lsp-vscode-extension/esbuild.js Outdated Show resolved Hide resolved
const path = require('path');
const polyfill = require('@esbuild-plugins/node-globals-polyfill');

const production = process.argv.includes('--production');
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that the argument is actually provided in the build scripts.

clients/cobol-lsp-vscode-extension/esbuild.js Outdated Show resolved Hide resolved
Fix the issue with "Assert" not being available as a package.
Remove Node Polyfill
Make buildWeb a separate job step for CI
Ran prettier to fix the linting issues.
Add upload step to buildWeb job
Co-authored-by: slavek-kucera <53339291+slavek-kucera@users.noreply.github.com>
@slavek-kucera
Copy link
Contributor

Please upgrade various ignore files...
.prettierignore - add .vscode-test-web
jest.config.js - add "<rootDir>/src/web","<rootDir>/.vscode-test","<rootDir>/.vscode-test-web",
src/test/suite/index.ts - exclude: ["**/test/**", ".vscode-test/**", ".vscode-test-web/**"],
tsconfig.json - "exclude": ["node_modules", "server", ".vscode-test", ".vscode-test-web"]

Implemented requested changes to TDD for it to account for the web extension.
Rename esbuild.js to avoid collision with just "esbuild".
Update .vscodeignore to exclude extension related javascript and map files from the extension/dist/ folders.
@slavek-kucera
Copy link
Contributor

Look through the build logs, I think that the web version also builds the desktop version in the vscode:prepublish step.
I also think that the browser entrypoint in the package.json will have to be deleted for the non-browser build and the main entrypoint in the web version.

Fix web build and package.
Add production flag to web build in prepackage
Updated .vscodeignore to remove the unneeded and unused exceptions to the ignore.
@slavek-kucera slavek-kucera merged commit 351d725 into eclipse-che4z:development Jun 14, 2024
17 checks passed
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.

4 participants