Implement graphql grammar #1949

vohoanglong0107 · 2024-03-01T14:23:16Z

Summary

Add the graphql grammar.

Part of #1927

Test Plan

It could be compiled.

netlify · 2024-03-01T14:23:36Z

✅ Deploy Preview for biomejs ready!

Name	Link
🔨 Latest commit	`8157aff`
🔍 Latest deploy log	https://app.netlify.com/sites/biomejs/deploys/65ffed0d4afd460008df2ddd
😎 Deploy Preview	https://deploy-preview-1949--biomejs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 99 (🟢 up 4 from production) Accessibility: 97 (no change from production) Best Practices: 100 (no change from production) SEO: 93 (no change from production) PWA: - View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

codspeed-hq · 2024-03-01T14:55:52Z

CodSpeed Performance Report

Merging #1949 will degrade performances by 8.48%

_{Comparing vohoanglong0107:feat-graphql (8157aff) with main (2edcd59)}

Summary

❌ 1 regressions
✅ 92 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	`main`	`vohoanglong0107:feat-graphql`	Change
❌	`db.json[cached]`	66.7 ms	72.9 ms	-8.48%

ematipico

Thank you @vohoanglong0107 for taking charge of this task. I left some comments.

The PR/grammar doesn't take into account any error recovery strategy. Is it on purpose? Do you plan to address it later? Here's some piece of documentation that should help: https://biomejs.dev/internals/architecture/#parser-and-cst

I plan to add more documentation on the subject, but it would nice to understand what you plan to do about it.

ematipico · 2024-03-19T10:43:16Z

crates/biome_graphql_syntax/Cargo.toml

+schemars    = { version = "0.8.10", optional = true }
+serde       = { version = "1.0.136", features = ["derive"], optional = true }


We should use workspace = true instead of using the the version

ematipico · 2024-03-19T10:47:51Z

xtask/codegen/graphql.ungram

+
+GraphqlDefinitionList = AnyGraphqlDefinition*
+
+AnyGraphqlDefinition =


When you define an Any* among the variants, you usually want to have a Bogus node. This isn't valid for each variant, but top-level variants should usually a bogus node.

ematipico · 2024-03-19T10:52:22Z

xtask/codegen/graphql.ungram

A couple of things that we should look after:

Mixed tabs and spaces. Let's fix that and use only tabs (you can ignore the legend).

Documentation: take a look at the other grammars, and you will notice how we try to document each node by writing an example of code, and highlighting the part of the code the node covers. This is very important to the project because we want other developers to understand your code quickly, especially if someone doesn't know the grammar. Please make sure to address that.

ematipico · 2024-03-19T10:59:32Z

crates/biome_graphql_syntax/src/lib.rs

+    /// Returns `true` for any contextual (await) or non-contextual keyword
+    #[inline]
+    pub const fn is_keyword(self) -> bool {
+        (self as u16) <= (GraphqlSyntaxKind::I_N_P_U_T_F_I_E_L_D_D_E_F_I_N_I_T_I_O_N_KW as u16)


I'm not sure what this is... but I_N_P_U_T_F_I_E_L_D_D_E_F_I_N_I_T_I_O_N_KW seems wrong to me.

Yeah this is the result of trying to differentiate between uppercase and lowercase keyword 😅

ematipico · 2024-03-19T11:00:17Z

xtask/codegen/src/js_kinds_src.rs

@@ -679,6 +679,7 @@ impl Field {
                    ("||", _) => "logical_or",
                    ("&&", _) => "logical_and",
                    ("$=", _) => "suffix",
+                    ("$", LanguageKind::Graphql) => "dollar",


Why was this change needed?

Graphql use $ for parameter, and without this there will be an invalid $_token

ematipico · 2024-03-19T11:02:32Z

xtask/codegen/src/generate_nodes.rs

@@ -976,7 +978,21 @@ pub(crate) fn token_kind_to_code(name: &str, language_kind: LanguageKind) -> Tok
    } else if kind_source.keywords.contains(&name) {
        // we need to replace "-" with "_" for the keywords
        // e.g. we have `color-profile` in css but it's an invalid ident in rust code
-        let token: TokenStream = name.replace('-', "_").parse().unwrap();
+        // also mark uppercase differently from lowercase
+        // e.g. "query" => "QUERY", "QUERY" => "Q_U_E_R_Y_"


Why do we need to mark upper-case keywords differently?
If so, is there a better way to tag them instead of using all underscores? Maybe with a postfix or something.

Graphql have both uppercase (QUERY) and lowercase keyword (query) so I want to have a way to differentiate them. I don't know what is the best way to do this, maybe adding a postfix if all the letters are uppercase?

Graphql have both uppercase (QUERY) and lowercase keyword (query) so I want to have a way to differentiate them. I don't know what is the best way to do this, maybe adding a postfix if all the letters are uppercase?

I think this could help the readability.

I would suggest QUERY_KW vs UPPER_QUERY_KW.

vohoanglong0107 · 2024-03-19T12:41:36Z

I didn't know about the error recovery strategy (especially about those bogus nodes 😅), so if you prefer I will include it in this PR. According to the linked docs, I should only need to add those bogus nodes in this PR right?

ematipico · 2024-03-20T08:39:14Z

We can add bogus nodes in another PR if you want, although that needs to be addressed. :)

ematipico · 2024-03-22T15:44:56Z

Heads up @vohoanglong0107, I pushed a commit to update few files.

knope.toml

Co-authored-by: unvalley <38400669+unvalley@users.noreply.github.com>

ematipico

Great work @vohoanglong0107 !

github-actions bot added the A-Tooling Area: internal tools label Mar 1, 2024

vohoanglong0107 force-pushed the feat-graphql branch from 4ef42ab to da7dd60 Compare March 5, 2024 00:04

vohoanglong0107 force-pushed the feat-graphql branch 5 times, most recently from 492a3e3 to a51d604 Compare March 18, 2024 10:58

vohoanglong0107 changed the title ~~Implement graphql parser~~ Implement graphql syntax Mar 18, 2024

vohoanglong0107 changed the title ~~Implement graphql syntax~~ Implement graphql grammar Mar 18, 2024

vohoanglong0107 force-pushed the feat-graphql branch from a51d604 to c6f0f00 Compare March 19, 2024 07:35

feat: graphql codegen

55875bf

vohoanglong0107 force-pushed the feat-graphql branch from c6f0f00 to 55875bf Compare March 19, 2024 07:36

vohoanglong0107 marked this pull request as ready for review March 19, 2024 08:39

vohoanglong0107 mentioned this pull request Mar 19, 2024

📎 GraphQL support #1927

Open

3 tasks

ematipico reviewed Mar 19, 2024

View reviewed changes

fix: mix tab and space

a653337

vohoanglong0107 force-pushed the feat-graphql branch from b66eda4 to a653337 Compare March 20, 2024 22:52

vohoanglong0107 and others added 6 commits March 22, 2024 12:14

fix: add comment for graphql urgram

769b213

fix: comma are optional

7a6a170

fix: use workspace version

3b50bc0

fix: add postfix for uppercase keyword

204b60f

Merge remote-tracking branch 'origin/main' into feat-graphql

56ae5c1

chore: update knope and version number

0b586f5

vohoanglong0107 added 2 commits March 23, 2024 01:23

feat: add bogus node

8385c66

fix: mark uppercase with UPPER_

bee279c

vohoanglong0107 requested review from arendjr and ematipico March 24, 2024 08:32

unvalley reviewed Mar 24, 2024

View reviewed changes

knope.toml Outdated Show resolved Hide resolved

Update knope.toml

8157aff

Co-authored-by: unvalley <38400669+unvalley@users.noreply.github.com>

ematipico reviewed Mar 27, 2024

View reviewed changes

ematipico approved these changes Mar 27, 2024

View reviewed changes

ematipico merged commit 7bf1861 into biomejs:main Mar 27, 2024
16 of 17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement graphql grammar #1949

Implement graphql grammar #1949

vohoanglong0107 commented Mar 1, 2024 •

edited by ematipico

netlify bot commented Mar 1, 2024 •

edited

codspeed-hq bot commented Mar 1, 2024 •

edited

ematipico left a comment

ematipico Mar 19, 2024

ematipico Mar 19, 2024

ematipico Mar 19, 2024

ematipico Mar 19, 2024

vohoanglong0107 Mar 19, 2024

ematipico Mar 19, 2024

vohoanglong0107 Mar 19, 2024

ematipico Mar 19, 2024

vohoanglong0107 Mar 19, 2024

ematipico Mar 20, 2024

arendjr Mar 22, 2024

vohoanglong0107 commented Mar 19, 2024

ematipico commented Mar 20, 2024

ematipico commented Mar 22, 2024

ematipico left a comment

		schemars = { version = "0.8.10", optional = true }
		serde = { version = "1.0.136", features = ["derive"], optional = true }


		GraphqlDefinitionList = AnyGraphqlDefinition*

		AnyGraphqlDefinition =

Implement graphql grammar #1949

Implement graphql grammar #1949

Conversation

vohoanglong0107 commented Mar 1, 2024 • edited by ematipico

Summary

Test Plan

netlify bot commented Mar 1, 2024 • edited

✅ Deploy Preview for biomejs ready!

codspeed-hq bot commented Mar 1, 2024 • edited

Merging #1949 will degrade performances by 8.48%

Summary

Benchmarks breakdown

ematipico left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vohoanglong0107 commented Mar 19, 2024

ematipico commented Mar 20, 2024

ematipico commented Mar 22, 2024

ematipico left a comment

Choose a reason for hiding this comment

vohoanglong0107 commented Mar 1, 2024 •

edited by ematipico

netlify bot commented Mar 1, 2024 •

edited

codspeed-hq bot commented Mar 1, 2024 •

edited