Skip to content

Conversation

@sunker
Copy link
Collaborator

@sunker sunker commented Jun 14, 2022

This PR composes and uses a custom redshift Monaco language that is based on the standard sql language from grafana experimental. This was made possible in this this PR in grafana experimental.

Part of #108
Fixes #153

@sunker sunker requested a review from a team as a code owner June 14, 2022 11:57
@sunker sunker requested review from andresmgot and asimpson and removed request for a team June 14, 2022 11:57
@github-actions
Copy link

github-actions bot commented Jun 14, 2022

Code coverage report for PR #154

Go TypeScript
main 45.9% 88%
PR 45.9% 87.73%
difference 0% -.27%

const redshiftLanguageDefinition: LanguageDefinition = {
id: 'redshift',
// TODO: Load language using code splitting instead: loader: () => import('./language'),
loader: () => Promise.resolve({ conf, language }),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This language is in no way heavy, but I would like to use code splitting here since that's the de facto way to load Monaco languages. However, when doing so webpack tries to resolve the script from the server root ([0].js), but it's placed in the public/plugins/grafana-redshift-datasource/0.js. Any idea on how to work around this @dprokop?

Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

My main question here is that if we are able to extend the existing monaco language for redshift so we don't need to re-define everything?


// Language based on standard SQL, but edited according to https://docs.aws.amazon.com/redshift/latest/dg/c_SQL_commands.html

export const logicalOperators = [
Copy link
Contributor

Choose a reason for hiding this comment

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

all this is already written in the official language for Redshift: https://github.com/microsoft/monaco-editor/blob/main/src/basic-languages/redshift/redshift.ts#L34

can we use it here?

@sunker
Copy link
Collaborator Author

sunker commented Jun 15, 2022

My main question here is that if we are able to extend the existing monaco language for redshift so we don't need to re-define everything?

Any language from the Monaco language registry can be used, but extending it is not possible. But we don't want to use the redshift language from the Monaco registry for a few reasons:

  • It's not official or actively maintained. Comparing for example the functions in the official language ref, there's a bunch of things missing in the Monaco language
  • Composing a new language based on the standard grafana sql language will give syntax highlighting for template variables and macros
  • Differentiating different operators using the SQLMonarchLanguage will enable a better autocomplete experience

Plan to update the redshift language definition so that it's at least somewhat close to official language ref in a future PR

@CLAassistant
Copy link

CLAassistant commented Jun 15, 2022

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

Got it, thanks for the explanation!

@sunker
Copy link
Collaborator Author

sunker commented Jun 16, 2022

@andresmgot any idea of what's causing the build error?

@andresmgot
Copy link
Contributor

It seems an issue with react 18: chakra-ui/chakra-ui#5896

Which got updated in your commit: d85b0b6

If you force "@types/react": "17.0.42", in the package.json as we do in grafana/grafana it should solve the issue but I don't know if it's a legit error that will bite us sooner or later

@sunker sunker force-pushed the custom-redshift-language branch from b7d3728 to 89ed1bc Compare June 16, 2022 08:55
@sunker sunker merged commit c4727e4 into main Jun 16, 2022
@sunker sunker deleted the custom-redshift-language branch June 16, 2022 09:47
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.

Define and load custom redshift sql language

3 participants