search: Remove MonacoQueryInput component and related code by fkling · Pull Request #46249 · sourcegraph/sourcegraph-public-snapshot

fkling · 2023-01-09T15:00:29Z

We've been using the CodeMirror query input version by default for a couple of months now. This diff

removes the MonacoQueryInput component and related code
renames LazyMonacoQueryInput to LazyQueryInput
removes the editorComponent prop from Lazy(Monaco)QueryInput

Not sure what to do about the feature flag setting, i.e. whether to remove "monaco" or add a note that it's not supported anymore. What do think?

@philipp-spiess: The VSCode extensions was still using the Monaco version. I think we have used the CodeMirror version long enough now that there shouldn't be a problem to switch it over.

@vovakulikov: The MonacoField (which I didn't rename) was passing a bunch of settings that only applied to the Monaco version. I think it's safe to just remove those but let me know if you there is anything specific I should test.

Test plan

TypeScript compiler.

App preview:

Web

Check out the client app preview documentation to learn more.

This commit removes the MonacoQueryInput component and renames "LazyMonacoQueryInput" to "LazyQueryInput".

sourcegraph-bot · 2023-01-09T15:02:15Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff 4ddf243...c12d351.

Notify	File(s)
@limitedmage	client/search-ui/src/index.ts client/search-ui/src/input/CodeMirrorQueryInput.tsx client/search-ui/src/input/LazyMonacoQueryInput.tsx client/search-ui/src/input/LazyQueryInput.module.scss client/search-ui/src/input/LazyQueryInput.test.tsx client/search-ui/src/input/LazyQueryInput.tsx client/search-ui/src/input/MonacoQueryInput.module.scss client/search-ui/src/input/MonacoQueryInput.story.tsx client/search-ui/src/input/MonacoQueryInput.tsx client/search-ui/src/input/QueryInput.ts client/search-ui/src/input/SearchBox.story.tsx client/search-ui/src/input/SearchBox.tsx client/search-ui/src/input/snapshots/LazyQueryInput.test.tsx.snap client/search-ui/src/input/useQueryIntelligence.ts client/web/src/enterprise/code-monitoring/components/FormTriggerArea.tsx client/web/src/integration/search-contexts.test.ts client/web/src/integration/search.test.ts client/web/src/search/home/SearchPageInput.tsx client/web/src/search/input/SearchNavbarItem.tsx
@philipp-spiess	client/jetbrains/webview/src/search/App.tsx client/jetbrains/webview/src/search/input/JetBrainsSearchBox.story.tsx client/jetbrains/webview/src/search/input/JetBrainsSearchBox.tsx
@vdavid	client/jetbrains/webview/src/search/App.tsx client/jetbrains/webview/src/search/input/JetBrainsSearchBox.story.tsx client/jetbrains/webview/src/search/input/JetBrainsSearchBox.tsx

vdavid

I reviewed the JetBrains part. The old code looks weird: it's like we passed "monaco" to the custom search box, but then disregarded that setting altogether and just used "codemirror6" anyway. Thanks for cleaning this up, your version removed this weird stuff.
Testing the search box is part of the release process for the JetBrains plugin, so at the next release we'll test if it works 100%, but the code LGTM.

I'm not approving the PR because I haven't checked the other parts, just saying that the JetBrains part looks good.

fkling · 2023-01-09T15:09:09Z

@vdavid Thank you! Yeah, that was weird, probably some left-over code while the CodeMirror version was still being evaluated.

sg-e2e-regression-test-bob · 2023-01-09T15:10:12Z

Bundle size report 📦

Initial size	Total size	Async size	Modules
-0.04% (-1.02 kb)	-0.36% (-51.78 kb) 🔽	-0.44% (-50.76 kb) 🔽	-0.55% (-4) 🔽

Look at the Statoscope report for a full comparison between the commits c12d351 and 94ffbdc or learn more.

Open explanation

Initial size is the size of the initial bundle (the one that is loaded when you open the page)
Total size is the size of the initial bundle + all the async loaded chunks
Async size is the size of all the async loaded chunks
Modules is the number of modules in the initial bundle

philipp-spiess

@philipp-spiess: The VSCode extensions was still using the Monaco version. I think we have used the CodeMirror version long enough now that there shouldn't be a problem to switch it over.

100% makes sense. Unfortunately I can't verify the VSCode build right now because of another issue (https://github.com/sourcegraph/sourcegraph/issues/46247) but I’m happy to fix it if anything breaks (but I’m sure it'll make things easier instead 😅 )

Not sure what to do about the feature flag setting, i.e. whether to remove "monaco" or add a note that it's not supported anymore. What do think?

It would be cool to remove it to avoid confusion. Is it possible to remove an option (or should we remove the whole feature-flag, something that seems possible now)

limitedmage

Looks great!! Left very minor comments.
IMO we should remove the experimental feature flag since it's no longer functional.

limitedmage · 2023-01-09T23:14:46Z

-    },
-}
-
 export interface MonacoFieldProps extends Omit<InputHTMLAttributes<HTMLInputElement>, 'value' | 'onChange' | 'onBlur'> {


Can we remove Monaco naming from this file and the component/props if it doesn't use Monaco anymore? Maybe to simply QueryField.

I'd leave that to the Code Insights team or at least do it in a separate diff.

No problems I can do it

fkling · 2023-01-10T07:50:01Z

@limitedmage, @philipp-spiess

IMO we should remove the experimental feature flag since it's no longer functional.

I guess it's possible that will use it for migrating the other Monaco instances... but it's true that we don't use right now. I'd be inclined to leave it as is right now, but if it's no problem to reintroduce it later then we can also remove it.

vovakulikov

Tested manually, code insights is fine, other parts should be fine as well but didn't conducted a deep manual testing beyond code insights creation UI

vovakulikov · 2023-01-10T18:31:52Z

-    },
-}
-
 export interface MonacoFieldProps extends Omit<InputHTMLAttributes<HTMLInputElement>, 'value' | 'onChange' | 'onBlur'> {


No problems I can do it

fkling · 2023-01-11T09:07:27Z

I've decided to also remove the setting to keep things clean.

search: Remove MonacoQueryInput component and related code

30ae0df

This commit removes the MonacoQueryInput component and renames "LazyMonacoQueryInput" to "LazyQueryInput".

fkling added the team/search-product Issues owned by the search product team label Jan 9, 2023

fkling requested review from limitedmage, philipp-spiess and vovakulikov January 9, 2023 15:00

cla-bot Bot added the cla-signed label Jan 9, 2023

Remove Monaco from integration tests

d17e48d

vdavid reviewed Jan 9, 2023

View reviewed changes

philipp-spiess approved these changes Jan 9, 2023

View reviewed changes

limitedmage approved these changes Jan 9, 2023

View reviewed changes

Simplify props interface

26c5eba

vovakulikov approved these changes Jan 10, 2023

View reviewed changes

Add changelog; update tests; remove setting

9bdee56

Update integration tests after removing setting

c12d351

fkling merged commit 0139807 into main Jan 11, 2023

fkling deleted the fkling/remove-monaco-query-input branch January 11, 2023 11:17

Conversation

fkling commented Jan 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test plan

App preview:

Uh oh!

sourcegraph-bot commented Jan 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vdavid left a comment

Choose a reason for hiding this comment

Uh oh!

fkling commented Jan 9, 2023

Uh oh!

sg-e2e-regression-test-bob commented Jan 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bundle size report 📦

Uh oh!

philipp-spiess left a comment

Choose a reason for hiding this comment

Uh oh!

limitedmage left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

limitedmage Jan 9, 2023

Choose a reason for hiding this comment

Uh oh!

fkling Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

vovakulikov Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

fkling commented Jan 10, 2023

Uh oh!

vovakulikov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vovakulikov Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

fkling commented Jan 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

fkling commented Jan 9, 2023 •

edited

Loading

sourcegraph-bot commented Jan 9, 2023 •

edited

Loading

sg-e2e-regression-test-bob commented Jan 9, 2023 •

edited

Loading