…grations
This PR adds v2t telemetry infrastructure to the Sourcegraph browser
extensions and native integrations code base.
## Test plan
- Tested locally using instructions at
https://github.com/sourcegraph/sourcegraph/tree/main/client/browser
- CI
## Changelog
---------
Co-authored-by: Dan Adler <5589410+dadlerj@users.noreply.github.com>
It seems many of our doc links for code hosts are broken in production
due to a url changed from external_services to code_hosts. I did a find
an replace to update all the ones I could find.
This runs playwright tests with bazel. This changes how the
app is served in the tests, specifically playwright will intercept all
network calls to the local server and serve the static assets directly
or serve root index.html file if nothing is matched.
---------
Co-authored-by: bahrmichael <michael.bahr@sourcegraph.com>
Co-authored-by: Jean-Hadrien Chabran <jh@chabran.fr>
Co-authored-by: Michael Bahr <1830132+bahrmichael@users.noreply.github.com>
Co-authored-by: Jean-Hadrien Chabran <jean-hadrien.chabran@sourcegraph.com>
Co-authored-by: Camden Cheek <camden@ccheek.com>
As a follow up to https://github.com/sourcegraph/sourcegraph/pull/61122 this commit updates the deprecated methods with the suggested replacements.
**Note**
This only migrates functions/methods that are replaced with something else, or whose deprecated call signature can easily be identified (e.g. `throwError(error)` -> `throwError(() => error)`). It's possible that there are more functions which deprecate a specific signature that we are using. I'll migrate those as I encounter them.
**Notes about `.toPromise`**
The instances of `.toPromise` converted here are all instances where the updated return value of `Promise<X|undefined>` did not produce a TS error (the ones with errors have been converted in #61122). However that doesn't mean that they can simply be replaced with `firstValueFrom`, `lastValueFrom` (these two methods throw errors when the source observable hasn't emitted a value before closing).
I update the callsites under two assumptions:
- Callsites that involve GraphQL requests will always emit a value and thus can be converted to using `lastValueFrom`/`firstValueFrom`.
- For other callsites we cannot make the assumption that the source observable emits before closing and thus they need a default value.
* Refactor URL helpers
We currently have quite a few URL helpers for which is not obvious how
they are supposed to be used. Additionally we often "prettify" certain
parts of URL when generating one but that's easy to forget.
This commit attempts to improve this situation in various ways:
- Reduce the number of helper functions. Instead provide a
`SourcegraphURL` class that should be used for parsing, manipulating
and converting a URL to a string.
- Rename helper functions that operate on `git:` URLs to make their
purpose clearer.
- Reduce the scope/visibility of certain helpers.
* Fix lint
* Fix lint issues
* Fix lint issues
* Fix lint issues
* Cleanup
* Fix lint issues
* More cleanup
* Fix lint issues
* Remove stray character
Codeintel hovercards are not appearing when the file page initially
loads. The codeintel CodeMirror extension is never created because
`getOrCreateCodeIntelAPI` never resolves.
That's because the `platform.settings` observable never completes, which
is necessary for `lastValueFrom` to work.
The simple fix would be to use `firstValueFrom`, but upon closer
inspection, `getOrCreateCodeIntelAPI` is only used by the codemirror
blob. I refactored it to avoid having to rely on platform context, which lets
us remove platform context from a bunch of places.
A side goal for the web rewrite is to leave the existing code base in a better state than before. I recently [added a hacky workaround](da5ddc99b6/client/web-sveltekit/vite.config.ts (L82-L101)) to make the Svelte version work properly with different rxjs versions. But the whole point of the rewrite is to not have to do these things anymore. So this is my attempt of upgrading rsjx in the main repo to v7.
I worked through the list of breaking changes in the [rxjs documentation](https://rxjs.dev/deprecations/breaking-changes) and fixed TypeScript issues to the best of my abilities.
Most notable changes:
- The custom `combineLatestOrDefault` operator was rewritten to avoid using rxjs internals, and the `.lift` method (side note: the corresponding tests do not cover all expected behavior, but issues had been caught through other tests)
- Where necessary `.toPromise()` was replaced with `lastValueFrom` or `firstValueFrom`. My assumption was that since we don't get runtime errors for the existing code, it's save to assume that the corresponding observables emit at least one value, i.e. `.toPromise()` did not return `undefined`. Only in some places I added a default value where it was easy to deduce what it should be.
- The generic type in `of()` was removed
- The generic type in `concat()` was removed
- `Subject.next` seemed to have allowed `undefined` to be passed even if the subject's types didn't allow that. If the subject's type couldn't be extended to include `undefined` I changed the code to not pass `undefined`.
- The generic type signature of `defaultIfEmpty` changed.
- Where possible I replaced `Subscribable` with `ObservableInput`, but we also have a copy of the old `Subscribable` interface in the `sourcegraph` package, and that makes things more complicated.
- I simplified unnecessary Promise/Observable interop where necessary.
A lot of the complex rxjs logic and usage of changed interfaces, such as `Subscribable`, is in extensions related code, which is not used in the web app anymore, but is still at least imported in the browser extensions code. Most of it is probably not used anymore, which makes the migration somewhat simpler.
We have a number of docs links in the product that point to the old doc site.
Method:
- Search the repo for `docs.sourcegraph.com`
- Exclude the `doc/` dir, all test fixtures, and `CHANGELOG.md`
- For each, replace `docs.sourcegraph.com` with `sourcegraph.com/docs`
- Navigate to the resulting URL ensuring it's not a dead link, updating the URL if necessary
Many of the URLs updated are just comments, but since I'm doing a manual audit of each URL anyways, I felt it was worth it to update these while I was at it.
Reimplement fetching & building code-intel-extension bundle as a bazel `http_archive` repository & refactored `js_run_binary`. This should allow the extensions bundle to be cached in the repository cache, instead of being affected by changing action environments
## Test plan
`bazel build //client/browser:code-intel-extensions` and `cd client/browser && pnpm run create-source-zip` run without error
* Add first raw implementation
* Add caching and multifunction for non-exclusive filters
* Add comment and refactor the dynamic search UI
* Fix useCachedSearchResults call
* Add icons to dynamic filter items
* Add filtering to dynamic filter section
* Fix typo
* Add Symbol filters section
* Add commit date filtes
* Fix ordering problem for static filter sections
* Add icons to symbol filters
* Add utility filters section
* Add footer link to doc
* Add author filters section
* Add avatars to author filter section
* Fix avatar styles
* Put new search filters UI behind feature flag
* Update bazel builds
* Fix eslint problems
* Fix titles and search content layout
* Fix streaming search results units
* Fix language icon after rebasing main
* Fix ts problems
* Remove left over comment from lang icon
* Extract dynamic filter item into separate component
* Fixes by PR review comments
* bazel configure
* Fix import
* Adjust language icon UI to the new icon system
* Update bazel build files
* Fix lint problems
* Update outdated snapshots tests
* Fix feature flag for filter button
* Fix repository integration tests mock
* Fix graphql type problem
* Update snapshots after main rebase
* reapply "switch from jest to vitest for faster, simpler tests (https://github.com/sourcegraph/sourcegraph/pull/57886)"
This was reverted in https://github.com/sourcegraph/sourcegraph/pull/58116 due to an issue with the browser tests.
* include fetch-mock
* fix flakiness
* rm mock that did not work in experimentalVmThreads
* fix
* timeout
* fixup
---------
Co-authored-by: William Bezuidenhout <william.bezuidenhout@sourcegraph.com>
When running vitests concurrently the target //client/browser:test fails
Revert "switch from jest to vitest for faster, simpler tests (#57886)"
This reverts commit ae5325e432.
Replaces our usage of jest with vitest. Also removes the babel transpiler. This simplifies our test configuration by a lot, makes tests run 10% faster, and makes further modernizations to our build/test stuff possible (such as using vite for frontend builds).
This removes some of the junit exporting for Buildkite, and the vitest bazel defs don't really cleanly implement bazel testing guidelines (like sharding). But vitest is only used for unit tests (all integration/e2e/regression tests have always run in mocha), so none of them are very slow anyway.
## Codemods for vitest imports
fastmod -e js,ts,tsx @jest/globals vitest client/ dev/release/
fastmod -e js,ts,tsx 'jest\.(\w+)\(' 'vi.$1(' client/ dev/release/
fastmod -e js,ts,tsx 'jest,' 'vi,' client/ dev/release/
fastmod -e js,ts,tsx 'jest }' 'vi }' client/ dev/release/
git diff --diff-filter=M --name-only | xargs pnpm exec prettier --write
swc is a very fast TypeScript transpiler.
Instead of using Babel for TypeScript transpilation in Bazel, we now use swc, which is much faster. We still use Babel in Jest (for tests), but in the future we intend to move away from Jest.
See https://docs.aspect.build/rulesets/aspect_rules_ts/docs/transpiler#swc-recommended for more information about using swc in Bazel, and https://swc.rs/ for general information about swc.
Brings something like https://github.com/sourcegraph/cody/pull/1192 to Sourcegraph's web client, injected as a new `telemetryRecorder` in `PlatformContext`. Because we want to use the only non-deprecated way to make GraphQL requests, Apollo clients, a global events recorder is not provided ([thread](https://sourcegraph.slack.com/archives/C01C3NCGD40/p1698376463350449)) - the shared one can only retrieved from `PlatformContext`.
In non-`web` platforms (`client/browser`) the recorder is currently set to a no-op recorder that errors upon use, as for this PR intended for backport, we only want to add some usages to `web` for Cody events.
To start off, this change adds some new usage of this on a rather out-of-the-way section `permissions-center`, tested manually below.
Closes https://github.com/sourcegraph/sourcegraph/issues/56920
Removed usage of gulp for running commands. Instead, we just use `package.json` scripts (that mostly invoke `ts-node -T ...`). The purpose of removing gulp is to remove a layer of duplication/indirection between the tasks we need to run and where they are defined.
The code generation tasks (GraphQL operations, CSS modules types, and schema/ JSON Schema types) no longer run in watch mode in local dev. If you make changes that require regeneration of this code, run `pnpm run generate` from the root. This is for simplicity and speed (they would run on many unrelated changes and slow down local dev).
* stub method to avoid unhandled exception in test from pretendRemote
* less flaky ActionItem test that checks condition before snapshotting
* rename *.{spec => test}.ts
This simplifies the test filename pattern.
* rename top-level tsconfig.all.json to tsconfig.json
* upgrade to pnpm 8.9.2
* avoid usage of jsdom.reconfigure
* more robust linkClickHandler.test.tsx
* make getBundleSizeStats.test.ts not need to use mocks
* extract createBarrier() to @sourcegraph/testing
* more robust fromObservableQuery.test.ts
* avoid jsdom.reconfigure
* do not attempt to request assets from assets.gitlab-static.net
fastmod -F assets.gitlab-static.net example.com client/browser/src/
* avoid "incorrect casing" for mocked React components
* use createBarrier
* fix SurveyToast mock GraphQL query
* fix classNames typo
This disables the bundling of code intel extensions with the firefox browser extension. The bundled files are minified, and I failed to run the build steps for the extensions locally (they're pretty out of date), so I can't really test the changes needed to build a non-minified version.
For now, rather than spending more time on extensions that live in a hazy state of support, this just removes the extensions from the firefox addon so we don't get removed from the store.
See https://github.com/typescript-eslint/typescript-eslint/pull/6754.
Also removes needless `.eslintrc.js` files (now that we can use the root `tsconfig.all.json` for linting and it's still fast enough).
Some of our eslint rules were unintentionally made ineffective in `client/web`, and this commit also re-enables them and in some cases suppresses the eslint warning where a fix is not urgent.
Previously bindings like `expect`, `describe`, `afterAll`, etc., were imported implicitly by Jest or Mocha. We should import them explicitly to reduce magic. (Also this makes it easier to migrate to another test runner in the future if we want.)
Our tracking of frontend (TypeScript) code coverage has decayed and was no longer being used. The Codecov report at https://app.codecov.io/gh/sourcegraph/sourcegraph significantly under-counts coverage, and what coverage tasks we did have were flaky in CI and relied on old libraries that have not been updated recently to keep with modern practices (such as `@storybook/addon-storyshots` only supporting Jest with `injectGlobals`).
From https://sourcegraph.slack.com/archives/C04MYFW01NV/p1698059032863009?thread_ts=1698054092.709309&cid=C04MYFW01NV, the proposal is to remove coverage tracking for now and prioritize getting tests to run really fast and smooth locally first, including upgrading our testing infra, and then reenable code coverage tracking.
* disable slow eslint rules, remove unused disable directives
Disabling these eslint rules makes saving significantly faster. These rules are not worthless, but they are usually ignored anyway, and I can't recall a specific instance when they would have caught a bug. I am proposing we disable them and then set a checkpoint in 14 days to rerun eslint with the rules enabled and see if they would have caught any bad practices. In the meantime, we will all benefit from instant saves (with eslint fixes) instead of waiting 3-5 seconds or more after each save in VS Code, which is destructive to productivity.
* upgrade eslint
Use [esbuild](https://esbuild.github.io/) instead of Webpack for builds of `client/web`, for faster builds (dev and prod) and greater dev-prod parity. This PR completely removes all use of Webpack in this repository.
`client/web` is the last build target that still uses Webpack; all others have been recently migrated to esbuild. Most devs here have been using esbuild for local dev of `client/web` for the last 6-12 months anyway. The change here is that now our production builds will be built by esbuild.
All sg commands, integration/e2e tests, etc., continue to work as-is. The bundlesize report will take a while to stabilize because the new build products use different filenames.
## Benchmarks
Running `pnpm run generate && time pnpm -C client/web run task:gulp webBuild` and taking the `time` output from the last command:
- Webpack: 62.5s
- esbuild: 6.7s
Note: This understates esbuild's victory for 2 reasons: (1) because esbuild is building both the main and embed entrypoints, whereas Webpack only builds the main entrypoint in this benchmark) and (2) because a lot of it is in the fixed startup time of `gulp`; esbuild incremental rebuilds during local dev only take ~1s.
## Notes
We no longer use Babel to produce web builds (we use esbuild), so we don't need any Babel plugins that optimize the output or improve browser compatibility. Right now, Babel is only used by Jest (for tests) and by Bazel as an intermediate step.
To pass multiple ignore patterns to Jest, you need to use `--testPathIgnorePatterns pattern1 --testPathIgnorePatterns pattern2 ...`. Otherwise, `--testPathIgnorePatterns pattern1 pattern2` treats `pattern2` as a test pattern to run, not to ignore.
The consequence of this is that some npm scripts and CI steps were running tests that they didn't need to run.
It turned out that we have two different ways of overriding feature
flags:
- `?feat=foo` overrides the feature flag on the server for this request
- `?feature-flag-key=foo&feature-flag-value=true` overrides the feature
flag on the client side only and persists the setting in local storage
This commit does three things:
1. It consolidates the two systems. `feature-flag-{key,value}` have been
removed in favor of `feat`. The client side logic will now use the
same parameter. An additional prefix has been added, `~` to indicate
the client to remove the override.
2. The header logic for GraphQL queries has been reworked to include
headers for overridden feature flags. This ensures that the server
uses the same overrides as the client.
3. The developer settings dialog and navbar entry now feature a reload
button which appends `flag` to the current URL for overridden feature
flags and reloads the page. Additionally the keyboard shortcut for
reloading the page (`Mod-r`) is hijacked to perform the same action.
NOTE: Unlike the first two, this functionality is only available in
development builds.
For reasons unknown I looked at the browser extension. This seems to fix the issues of showing the "open in sourcegraph" button in the file header.
I modified the E2E test to make hovercard testing optional so that we can at least test the toolbar button.
Addendum: I also updated the pipline generate to run browser extension tests when code inside client/browser/ changes.
