From e9691164b9f321cc9f34c971bba66fc9e7a8f9f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20Wa=C5=82ach?= <35966385+PatrykWalach@users.noreply.github.com> Date: Tue, 12 Nov 2024 20:50:29 +0100 Subject: [PATCH 01/18] fix error with hot module replacement --- .../src/useCachedResponsivePrecommitValue.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts b/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts index cbe3c86f2..ee5460a5f 100644 --- a/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts +++ b/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts @@ -51,6 +51,10 @@ export function useCachedResponsivePrecommitValue( const lastCommittedParentCache = useRef | null>(null); useEffect(() => { + if (lastCommittedParentCache.current === parentCache) { + return; + } + lastCommittedParentCache.current = parentCache; // On commit, cacheItem may be disposed, because during the render phase, // we only temporarily retained the item, and the temporary retain could have From 25ba84c389ede92ca597fa256283fefe3aa39781 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20Wa=C5=82ach?= <35966385+PatrykWalach@users.noreply.github.com> Date: Tue, 12 Nov 2024 21:18:15 +0100 Subject: [PATCH 02/18] update tests --- .../isograph-react-disposable-state/package.json | 1 + .../useCachedResponsivePrecommitValue.test.tsx | 7 ++++--- pnpm-lock.yaml | 16 +++++++++++++--- 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/libs/isograph-react-disposable-state/package.json b/libs/isograph-react-disposable-state/package.json index 865cbb8c0..c0d5f819c 100644 --- a/libs/isograph-react-disposable-state/package.json +++ b/libs/isograph-react-disposable-state/package.json @@ -25,6 +25,7 @@ }, "devDependencies": { "@types/react": "18.3.1", + "@types/react-test-renderer": "^18.3.0", "react-test-renderer": "^18.2.0", "typescript": "5.6.3" }, diff --git a/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.test.tsx b/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.test.tsx index f743da216..d8415b931 100644 --- a/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.test.tsx +++ b/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.test.tsx @@ -2,7 +2,7 @@ import { describe, test, vi, expect, assert } from 'vitest'; import { ParentCache } from './ParentCache'; import { ItemCleanupPair } from '@isograph/disposable-types'; import { useCachedResponsivePrecommitValue } from './useCachedResponsivePrecommitValue'; -import React from 'react'; +import React, { StrictMode, type ReactElement } from 'react'; import { create } from 'react-test-renderer'; import { CacheItem, CacheItemState } from './CacheItem'; @@ -51,9 +51,10 @@ function promiseAndResolver() { // The fact that sometimes we need to render in concurrent mode and sometimes // not is a bit worrisome. -async function awaitableCreate(Component, isConcurrent) { +async function awaitableCreate(Component: ReactElement, isConcurrent) { const element = create( - Component, + {Component}, + // @ts-expect-error isConcurrent ? { unstable_isConcurrent: true } : undefined, ); await shortPromise(); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 234c80957..98e0178d4 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -364,6 +364,9 @@ importers: '@types/react': specifier: 18.3.1 version: 18.3.1 + '@types/react-test-renderer': + specifier: ^18.3.0 + version: 18.3.0 react-test-renderer: specifier: ^18.2.0 version: 18.3.1(react@18.3.1) @@ -2267,6 +2270,9 @@ packages: '@types/react-router@5.1.20': resolution: {integrity: sha512-jGjmu/ZqS7FjSH6owMcD5qpq19+1RS9DeVRqfl1FeBMxTDQAGwlMWOcs52NDoXaNKyG3d1cYQFMs9rCrb88o9Q==} + '@types/react-test-renderer@18.3.0': + resolution: {integrity: sha512-HW4MuEYxfDbOHQsVlY/XtOvNHftCVEPhJF2pQXXwcUiUF+Oyb0usgp48HSgpK5rt8m9KZb22yqOeZm+rrVG8gw==} + '@types/react-transition-group@4.4.11': resolution: {integrity: sha512-RM05tAniPZ5DZPzzNFP+DmrcOdD0efDUxMy3145oljWSl3x9ZV5vhme98gTxFrj2lhXvmGNnUiuDyJgY9IKkNA==} @@ -10280,6 +10286,10 @@ snapshots: '@types/history': 4.7.11 '@types/react': 18.3.1 + '@types/react-test-renderer@18.3.0': + dependencies: + '@types/react': 18.3.1 + '@types/react-transition-group@4.4.11': dependencies: '@types/react': 18.3.1 @@ -12087,7 +12097,7 @@ snapshots: eslint: 8.39.0 eslint-import-resolver-node: 0.3.9 eslint-import-resolver-typescript: 3.6.3(@typescript-eslint/parser@5.62.0(eslint@8.39.0)(typescript@5.6.3))(eslint-import-resolver-node@0.3.9)(eslint-plugin-import@2.30.0(eslint@8.39.0))(eslint@8.39.0) - eslint-plugin-import: 2.30.0(@typescript-eslint/parser@5.62.0(eslint@8.39.0)(typescript@5.6.3))(eslint-import-resolver-typescript@3.6.3(@typescript-eslint/parser@5.62.0(eslint@8.39.0)(typescript@5.6.3))(eslint-import-resolver-node@0.3.9)(eslint-plugin-import@2.30.0(eslint@8.39.0))(eslint@8.39.0))(eslint@8.39.0) + eslint-plugin-import: 2.30.0(@typescript-eslint/parser@5.62.0(eslint@8.39.0)(typescript@5.6.3))(eslint-import-resolver-typescript@3.6.3)(eslint@8.39.0) eslint-plugin-jsx-a11y: 6.10.0(eslint@8.39.0) eslint-plugin-react: 7.36.1(eslint@8.39.0) eslint-plugin-react-hooks: 4.6.2(eslint@8.39.0) @@ -12137,7 +12147,7 @@ snapshots: is-bun-module: 1.2.1 is-glob: 4.0.3 optionalDependencies: - eslint-plugin-import: 2.30.0(@typescript-eslint/parser@5.62.0(eslint@8.39.0)(typescript@5.6.3))(eslint-import-resolver-typescript@3.6.3(@typescript-eslint/parser@5.62.0(eslint@8.39.0)(typescript@5.6.3))(eslint-import-resolver-node@0.3.9)(eslint-plugin-import@2.30.0(eslint@8.39.0))(eslint@8.39.0))(eslint@8.39.0) + eslint-plugin-import: 2.30.0(@typescript-eslint/parser@5.62.0(eslint@8.39.0)(typescript@5.6.3))(eslint-import-resolver-typescript@3.6.3)(eslint@8.39.0) transitivePeerDependencies: - '@typescript-eslint/parser' - eslint-import-resolver-node @@ -12185,7 +12195,7 @@ snapshots: transitivePeerDependencies: - supports-color - eslint-plugin-import@2.30.0(@typescript-eslint/parser@5.62.0(eslint@8.39.0)(typescript@5.6.3))(eslint-import-resolver-typescript@3.6.3(@typescript-eslint/parser@5.62.0(eslint@8.39.0)(typescript@5.6.3))(eslint-import-resolver-node@0.3.9)(eslint-plugin-import@2.30.0(eslint@8.39.0))(eslint@8.39.0))(eslint@8.39.0): + eslint-plugin-import@2.30.0(@typescript-eslint/parser@5.62.0(eslint@8.39.0)(typescript@5.6.3))(eslint-import-resolver-typescript@3.6.3)(eslint@8.39.0): dependencies: '@rtsao/scc': 1.1.0 array-includes: 3.1.8 From 7d4628dcf10476c39658f956fa957f2abe18ddf5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20Wa=C5=82ach?= <35966385+PatrykWalach@users.noreply.github.com> Date: Thu, 14 Nov 2024 15:07:00 +0100 Subject: [PATCH 03/18] fix cleanup --- .../src/useDisposableState.ts | 24 ++++++++++++------- .../src/useLazyDisposableState.test.tsx | 9 +++++-- .../src/useLazyDisposableState.ts | 14 +++++++---- .../src/useUpdatableDisposableState.test.tsx | 5 ++-- 4 files changed, 36 insertions(+), 16 deletions(-) diff --git a/libs/isograph-react-disposable-state/src/useDisposableState.ts b/libs/isograph-react-disposable-state/src/useDisposableState.ts index 46a63fe81..81e18fffe 100644 --- a/libs/isograph-react-disposable-state/src/useDisposableState.ts +++ b/libs/isograph-react-disposable-state/src/useDisposableState.ts @@ -22,7 +22,6 @@ export function useDisposableState( const preCommitItem = useCachedResponsivePrecommitValue( parentCache, (pair) => { - itemCleanupPairRef.current?.[1](); itemCleanupPairRef.current = pair; }, ); @@ -47,14 +46,23 @@ export function useDisposableState( [stateFromDisposableStateHook], ); - useEffect(function cleanupItemCleanupPairRefIfSetStateNotCalled() { - return () => { - if (itemCleanupPairRef.current !== null) { - itemCleanupPairRef.current[1](); - itemCleanupPairRef.current = null; + const lastCommittedParentCache = useRef | null>(null); + + useEffect( + function cleanupItemCleanupPairRefIfSetStateNotCalled() { + if (lastCommittedParentCache.current === parentCache) { + return; } - }; - }, []); + lastCommittedParentCache.current = parentCache; + // capture last set pair in a variable + const current = itemCleanupPairRef.current; + return () => { + // current is a stale variable + current?.[1](); + }; + }, + [parentCache], + ); // Safety: we can be in one of three states. Pre-commit, in which case // preCommitItem is assigned, post-commit but before setState has been diff --git a/libs/isograph-react-disposable-state/src/useLazyDisposableState.test.tsx b/libs/isograph-react-disposable-state/src/useLazyDisposableState.test.tsx index 9d206cd63..38f4c3679 100644 --- a/libs/isograph-react-disposable-state/src/useLazyDisposableState.test.tsx +++ b/libs/isograph-react-disposable-state/src/useLazyDisposableState.test.tsx @@ -1,5 +1,5 @@ import { ItemCleanupPair } from '@isograph/disposable-types'; -import React, { useEffect, useState } from 'react'; +import React, { StrictMode, useEffect, useState } from 'react'; import { create } from 'react-test-renderer'; import { describe, expect, test, vi } from 'vitest'; import { ParentCache } from './ParentCache'; @@ -55,7 +55,12 @@ describe('useLazyDisposableState', async () => { return null; } - const root = create(, { unstable_isConcurrent: true }); + const root = create( + + + , + { unstable_isConcurrent: true }, + ); await committed.promise; expect(cache1.disposeItem).toHaveBeenCalled(); expect(cache1.cache.factory).toHaveBeenCalledOnce(); diff --git a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts index 5c251fd32..38da37c4c 100644 --- a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts +++ b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts @@ -23,18 +23,24 @@ export function useLazyDisposableState( state: T; } { const itemCleanupPairRef = useRef | null>(null); - const preCommitItem = useCachedResponsivePrecommitValue( parentCache, (pair) => { - itemCleanupPairRef.current?.[1](); itemCleanupPairRef.current = pair; }, ); + const lastCommittedParentCache = useRef | null>(null); useEffect(() => { + if (lastCommittedParentCache.current === parentCache) { + return; + } + lastCommittedParentCache.current = parentCache; + // capture last set pair in a variable + const current = itemCleanupPairRef.current; return () => { - const cleanupFn = itemCleanupPairRef.current?.[1]; + // current is a stale variable + const cleanupFn = current?.[1]; // TODO confirm useEffect is called in order. if (cleanupFn == null) { throw new Error( @@ -43,7 +49,7 @@ export function useLazyDisposableState( } return cleanupFn(); }; - }, []); + }, [parentCache]); const returnedItem = preCommitItem?.state ?? itemCleanupPairRef.current?.[0]; diff --git a/libs/isograph-react-disposable-state/src/useUpdatableDisposableState.test.tsx b/libs/isograph-react-disposable-state/src/useUpdatableDisposableState.test.tsx index e74c132c8..48459175e 100644 --- a/libs/isograph-react-disposable-state/src/useUpdatableDisposableState.test.tsx +++ b/libs/isograph-react-disposable-state/src/useUpdatableDisposableState.test.tsx @@ -1,5 +1,5 @@ import { describe, test, vi, expect } from 'vitest'; -import React from 'react'; +import React, { StrictMode } from 'react'; import { create } from 'react-test-renderer'; import { useUpdatableDisposableState, @@ -45,7 +45,8 @@ function promiseAndResolver() { // not is a bit worrisome. async function awaitableCreate(Component, isConcurrent) { const element = create( - Component, + {Component}, + isConcurrent ? { unstable_isConcurrent: true } : undefined, ); await shortPromise(); From 1ddc73b041c26543e2d6abe467a7eb42ed782d64 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20Wa=C5=82ach?= <35966385+PatrykWalach@users.noreply.github.com> Date: Thu, 14 Nov 2024 15:26:30 +0100 Subject: [PATCH 04/18] don't use stale itemCleanupPairRef --- .../src/useDisposableState.ts | 8 ++++---- .../src/useLazyDisposableState.ts | 6 ++---- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/libs/isograph-react-disposable-state/src/useDisposableState.ts b/libs/isograph-react-disposable-state/src/useDisposableState.ts index 81e18fffe..d5e979459 100644 --- a/libs/isograph-react-disposable-state/src/useDisposableState.ts +++ b/libs/isograph-react-disposable-state/src/useDisposableState.ts @@ -54,11 +54,11 @@ export function useDisposableState( return; } lastCommittedParentCache.current = parentCache; - // capture last set pair in a variable - const current = itemCleanupPairRef.current; + return () => { - // current is a stale variable - current?.[1](); + if (itemCleanupPairRef.current !== null) { + itemCleanupPairRef.current[1](); + } }; }, [parentCache], diff --git a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts index 38da37c4c..42a857e65 100644 --- a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts +++ b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts @@ -36,11 +36,9 @@ export function useLazyDisposableState( return; } lastCommittedParentCache.current = parentCache; - // capture last set pair in a variable - const current = itemCleanupPairRef.current; + return () => { - // current is a stale variable - const cleanupFn = current?.[1]; + const cleanupFn = itemCleanupPairRef.current?.[1]; // TODO confirm useEffect is called in order. if (cleanupFn == null) { throw new Error( From 07de13613128c06fc1f180b83a7fd5301f89ee1b Mon Sep 17 00:00:00 2001 From: Robert Balicki Date: Sat, 4 Jan 2025 01:44:51 +0900 Subject: [PATCH 05/18] Rename TextSource -> RelativeTextSource and SourceFileName to RelativePathToSourceFile - These aren't relative, yet! But they will be after a future commit. --- crates/common_lang_types/src/location.rs | 14 ++-- crates/common_lang_types/src/span.rs | 6 +- .../common_lang_types/src/string_key_types.rs | 2 +- .../graphql_schema_parser/src/parse_schema.rs | 64 +++++++++---------- .../src/isograph_literals.rs | 15 +++-- crates/isograph_compiler/src/source_files.rs | 19 +++--- .../src/parse_iso_literal.rs | 32 +++++----- .../isograph_lsp/src/semantic_tokens/mod.rs | 4 +- .../src/process_client_field_declaration.rs | 6 +- .../isograph_schema/src/unvalidated_schema.rs | 4 +- .../src/validate_entrypoint.rs | 8 +-- .../tests/tests/directives_deserialization.rs | 4 +- 12 files changed, 92 insertions(+), 86 deletions(-) diff --git a/crates/common_lang_types/src/location.rs b/crates/common_lang_types/src/location.rs index fe62f7d72..1ee27d812 100644 --- a/crates/common_lang_types/src/location.rs +++ b/crates/common_lang_types/src/location.rs @@ -2,7 +2,7 @@ use std::{error::Error, fmt}; use intern::Lookup; -use crate::{text_with_carats::text_with_carats, SourceFileName, Span, WithSpan}; +use crate::{text_with_carats::text_with_carats, RelativePathToSourceFile, Span, WithSpan}; /// A source, which consists of a filename, and an optional span /// indicating the subset of the file which corresponds to the @@ -12,12 +12,12 @@ use crate::{text_with_carats::text_with_carats, SourceFileName, Span, WithSpan}; /// as this will probably mean that sources are more reusable /// during watch mode. #[derive(Copy, Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Hash)] -pub struct TextSource { - pub path: SourceFileName, +pub struct RelativeTextSource { + pub path: RelativePathToSourceFile, pub span: Option, } -impl TextSource { +impl RelativeTextSource { pub fn read_to_string(&self) -> (&str, String) { // TODO maybe intern these or somehow avoid reading a bajillion times. // This is especially important for when we display many errors. @@ -34,7 +34,7 @@ impl TextSource { #[derive(Copy, Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Hash)] pub struct EmbeddedLocation { - pub text_source: TextSource, + pub text_source: RelativeTextSource, /// The span is relative to the Source's span, not to the /// entire source file. pub span: Span, @@ -65,7 +65,7 @@ impl Location { pub fn generated() -> Self { Location::Generated } - pub fn new(text_source: TextSource, span: Span) -> Self { + pub fn new(text_source: RelativeTextSource, span: Span) -> Self { Location::Embedded(EmbeddedLocation::new(text_source, span)) } @@ -77,7 +77,7 @@ impl Location { } } impl EmbeddedLocation { - pub fn new(text_source: TextSource, span: Span) -> Self { + pub fn new(text_source: RelativeTextSource, span: Span) -> Self { EmbeddedLocation { text_source, span } } } diff --git a/crates/common_lang_types/src/span.rs b/crates/common_lang_types/src/span.rs index 565e2c496..d6dc2f555 100644 --- a/crates/common_lang_types/src/span.rs +++ b/crates/common_lang_types/src/span.rs @@ -1,6 +1,6 @@ use std::{fmt, ops::Range}; -use crate::{EmbeddedLocation, Location, TextSource, WithEmbeddedLocation, WithLocation}; +use crate::{EmbeddedLocation, Location, RelativeTextSource, WithEmbeddedLocation, WithLocation}; // Invariant: end >= start #[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash, Debug)] @@ -101,11 +101,11 @@ impl WithSpan { } } - pub fn to_with_location(self, text_source: TextSource) -> WithLocation { + pub fn to_with_location(self, text_source: RelativeTextSource) -> WithLocation { WithLocation::new(self.item, Location::new(text_source, self.span)) } - pub fn to_with_embedded_location(self, text_source: TextSource) -> WithEmbeddedLocation { + pub fn to_with_embedded_location(self, text_source: RelativeTextSource) -> WithEmbeddedLocation { WithEmbeddedLocation::new(self.item, EmbeddedLocation::new(text_source, self.span)) } } diff --git a/crates/common_lang_types/src/string_key_types.rs b/crates/common_lang_types/src/string_key_types.rs index c8c8ab941..45c932106 100644 --- a/crates/common_lang_types/src/string_key_types.rs +++ b/crates/common_lang_types/src/string_key_types.rs @@ -111,7 +111,7 @@ string_key_newtype!(IsographDirectiveName); string_key_newtype!(FieldArgumentName); -string_key_newtype!(SourceFileName); +string_key_newtype!(RelativePathToSourceFile); string_key_newtype!(ArtifactFileType); diff --git a/crates/graphql_schema_parser/src/parse_schema.rs b/crates/graphql_schema_parser/src/parse_schema.rs index 7e421226b..75f26d1c8 100644 --- a/crates/graphql_schema_parser/src/parse_schema.rs +++ b/crates/graphql_schema_parser/src/parse_schema.rs @@ -2,7 +2,7 @@ use std::{ops::ControlFlow, str::FromStr}; use common_lang_types::{ DescriptionValue, EnumLiteralValue, GraphQLInterfaceTypeName, GraphQLObjectTypeName, Span, - StringLiteralValue, TextSource, WithLocation, WithSpan, + StringLiteralValue, RelativeTextSource, WithLocation, WithSpan, }; use graphql_syntax::TokenKind; use intern::{ @@ -31,7 +31,7 @@ use super::{ pub fn parse_schema( source: &str, - text_source: TextSource, + text_source: RelativeTextSource, ) -> ParseResult { let mut tokens = PeekableLexer::new(source); @@ -40,7 +40,7 @@ pub fn parse_schema( fn parse_type_system_document( tokens: &mut PeekableLexer, - text_source: TextSource, + text_source: RelativeTextSource, ) -> ParseResult { let mut type_system_definitions = vec![]; while !tokens.reached_eof() { @@ -52,7 +52,7 @@ fn parse_type_system_document( pub fn parse_schema_extensions( source: &str, - text_source: TextSource, + text_source: RelativeTextSource, ) -> ParseResult { let mut tokens = PeekableLexer::new(source); @@ -61,7 +61,7 @@ pub fn parse_schema_extensions( fn parse_type_system_extension_document( tokens: &mut PeekableLexer, - text_source: TextSource, + text_source: RelativeTextSource, ) -> ParseResult { let mut definitions_or_extensions = vec![]; while !tokens.reached_eof() { @@ -92,7 +92,7 @@ fn parse_type_system_extension_document( fn parse_type_system_extension( tokens: &mut PeekableLexer, - text_source: TextSource, + text_source: RelativeTextSource, ) -> ParseResult> { let extension = tokens.with_span(|tokens| { let identifier = tokens @@ -123,7 +123,7 @@ fn parse_type_system_extension( fn parse_type_system_definition( tokens: &mut PeekableLexer, - text_source: TextSource, + text_source: RelativeTextSource, ) -> ParseResult> { let definition = tokens.with_span(|tokens| { let description = parse_optional_description(tokens); @@ -163,7 +163,7 @@ fn parse_type_system_definition( fn parse_object_type_definition( tokens: &mut PeekableLexer, description: Option>, - text_source: TextSource, + text_source: RelativeTextSource, ) -> ParseResult { let name = tokens .parse_string_key_type(TokenKind::Identifier) @@ -186,7 +186,7 @@ fn parse_object_type_definition( /// The state of the PeekableLexer is that it has processed the "type" keyword fn parse_object_type_extension( tokens: &mut PeekableLexer, - text_source: TextSource, + text_source: RelativeTextSource, ) -> ParseResult { let name = tokens .parse_string_key_type(TokenKind::Identifier) @@ -209,7 +209,7 @@ fn parse_object_type_extension( fn parse_interface_type_definition( tokens: &mut PeekableLexer, description: Option>, - text_source: TextSource, + text_source: RelativeTextSource, ) -> ParseResult { let name = tokens .parse_string_key_type(TokenKind::Identifier) @@ -232,7 +232,7 @@ fn parse_interface_type_definition( fn parse_input_object_type_definition( tokens: &mut PeekableLexer, description: Option>, - text_source: TextSource, + text_source: RelativeTextSource, ) -> ParseResult { let name = tokens .parse_string_key_type(TokenKind::Identifier) @@ -260,7 +260,7 @@ fn parse_input_object_type_definition( fn parse_directive_definition( tokens: &mut PeekableLexer, description: Option>, - text_source: TextSource, + text_source: RelativeTextSource, ) -> ParseResult { let _at = tokens.parse_token_of_kind(TokenKind::At); let name = tokens @@ -337,7 +337,7 @@ fn parse_directive_location( fn parse_enum_definition( tokens: &mut PeekableLexer, description: Option>, - text_source: TextSource, + text_source: RelativeTextSource, ) -> ParseResult { let name = tokens .parse_string_key_type(TokenKind::Identifier) @@ -358,7 +358,7 @@ fn parse_enum_definition( fn parse_enum_value_definitions( tokens: &mut PeekableLexer, - text_source: TextSource, + text_source: RelativeTextSource, ) -> ParseResult>> { parse_optional_enclosed_items( tokens, @@ -371,7 +371,7 @@ fn parse_enum_value_definitions( fn parse_enum_value_definition( tokens: &mut PeekableLexer, - text_source: TextSource, + text_source: RelativeTextSource, ) -> ParseResult> { tokens.with_span(|tokens| { let description = parse_optional_description(tokens); @@ -404,7 +404,7 @@ fn parse_enum_value_definition( fn parse_union_definition( tokens: &mut PeekableLexer, description: Option>, - text_source: TextSource, + text_source: RelativeTextSource, ) -> ParseResult { let name = tokens .parse_string_key_type(TokenKind::Identifier) @@ -429,7 +429,7 @@ fn parse_union_definition( fn parse_union_member_types( tokens: &mut PeekableLexer, - text_source: TextSource, + text_source: RelativeTextSource, ) -> ParseResult>> { // This is a no-op if the token kind doesn't match, so effectively // this is an optional pipe @@ -456,7 +456,7 @@ fn parse_union_member_types( fn parse_schema_definition( tokens: &mut PeekableLexer, description: Option>, - text_source: TextSource, + text_source: RelativeTextSource, ) -> ParseResult { let directives = parse_constant_directives(tokens, text_source)?; @@ -515,7 +515,7 @@ fn reassign_or_error( fn parse_root_operation_type( tokens: &mut PeekableLexer, - text_source: TextSource, + text_source: RelativeTextSource, ) -> ParseResult<( WithSpan, WithLocation, @@ -554,7 +554,7 @@ fn parse_root_operation_type( fn parse_scalar_type_definition( tokens: &mut PeekableLexer, description: Option>, - text_source: TextSource, + text_source: RelativeTextSource, ) -> ParseResult { let name = tokens .parse_string_key_type(TokenKind::Identifier) @@ -573,7 +573,7 @@ fn parse_scalar_type_definition( /// The state of the PeekableLexer is that we have not parsed the "implements" keyword. fn parse_implements_interfaces_if_present( tokens: &mut PeekableLexer, - text_source: TextSource, + text_source: RelativeTextSource, ) -> ParseResult>> { if tokens.parse_matching_identifier("implements").is_ok() { let interfaces = parse_interfaces(tokens, text_source)?; @@ -594,7 +594,7 @@ fn parse_implements_interfaces_if_present( /// with only "Foo", no directives and no fields was successfully parsed. fn parse_interfaces( tokens: &mut PeekableLexer, - text_source: TextSource, + text_source: RelativeTextSource, ) -> ParseResult>> { let _optional_ampersand = tokens.parse_token_of_kind(TokenKind::Ampersand); @@ -618,7 +618,7 @@ fn parse_interfaces( fn parse_constant_directives( tokens: &mut PeekableLexer, - text_source: TextSource, + text_source: RelativeTextSource, ) -> ParseResult>> { let mut directives = vec![]; while tokens.parse_token_of_kind(TokenKind::At).is_ok() { @@ -636,7 +636,7 @@ fn parse_constant_directives( // Parse constant arguments passed to a directive used in a schema definition. fn parse_optional_constant_arguments>( tokens: &mut PeekableLexer, - text_source: TextSource, + text_source: RelativeTextSource, ) -> ParseResult>> { if tokens.parse_token_of_kind(TokenKind::OpenParen).is_ok() { let first_name_value_pair = parse_constant_name_value_pair( @@ -665,7 +665,7 @@ fn parse_optional_constant_arguments>( fn parse_constant_name_value_pair, TValue>( tokens: &mut PeekableLexer, parse_value: impl Fn(&mut PeekableLexer) -> ParseResult>, - text_source: TextSource, + text_source: RelativeTextSource, ) -> ParseResult> { let name = tokens .parse_string_key_type(TokenKind::Identifier) @@ -681,7 +681,7 @@ fn parse_constant_name_value_pair, TValue>( fn parse_constant_value( tokens: &mut PeekableLexer, - text_source: TextSource, + text_source: RelativeTextSource, ) -> ParseResult> { from_control_flow(|| { to_control_flow(|| { @@ -833,7 +833,7 @@ fn from_control_flow(control_flow: impl FnOnce() -> ControlFlow) -> fn parse_optional_fields( tokens: &mut PeekableLexer<'_>, - text_source: TextSource, + text_source: RelativeTextSource, ) -> ParseResult>> { let brace = tokens.parse_token_of_kind(TokenKind::OpenBrace); if brace.is_err() { @@ -851,7 +851,7 @@ fn parse_optional_fields( fn parse_field( tokens: &mut PeekableLexer<'_>, - text_source: TextSource, + text_source: RelativeTextSource, ) -> ParseResult> { let with_span = tokens.with_span(|tokens| { let description = parse_optional_description(tokens); @@ -948,10 +948,10 @@ fn parse_type_annotation>( fn parse_optional_enclosed_items<'a, T>( tokens: &mut PeekableLexer<'a>, - text_source: TextSource, + text_source: RelativeTextSource, open_token: TokenKind, close_token: TokenKind, - mut parse: impl FnMut(&mut PeekableLexer<'a>, TextSource) -> ParseResult>, + mut parse: impl FnMut(&mut PeekableLexer<'a>, RelativeTextSource) -> ParseResult>, ) -> ParseResult>> { let paren = tokens.parse_token_of_kind(open_token); @@ -970,7 +970,7 @@ fn parse_optional_enclosed_items<'a, T>( fn parse_argument_definition( tokens: &mut PeekableLexer<'_>, - text_source: TextSource, + text_source: RelativeTextSource, ) -> ParseResult> { tokens.with_span(|tokens| { let description = parse_optional_description(tokens); @@ -997,7 +997,7 @@ fn parse_argument_definition( fn parse_optional_constant_default_value( tokens: &mut PeekableLexer<'_>, - text_source: TextSource, + text_source: RelativeTextSource, ) -> ParseResult>> { let equal = tokens.parse_token_of_kind(TokenKind::Equals); if equal.is_err() { diff --git a/crates/isograph_compiler/src/isograph_literals.rs b/crates/isograph_compiler/src/isograph_literals.rs index 6c4fb2c26..530d95e6f 100644 --- a/crates/isograph_compiler/src/isograph_literals.rs +++ b/crates/isograph_compiler/src/isograph_literals.rs @@ -4,7 +4,9 @@ use std::{ path::{Path, PathBuf}, }; -use common_lang_types::{FilePath, Location, SourceFileName, Span, TextSource, WithLocation}; +use common_lang_types::{ + FilePath, Location, RelativePathToSourceFile, RelativeTextSource, Span, WithLocation, +}; use intern::string_key::Intern; use isograph_lang_parser::{ parse_iso_literal, IsoLiteralExtractionResult, IsographLiteralParseError, @@ -110,8 +112,8 @@ pub(crate) fn read_and_parse_iso_literals( canonicalized_root_path: &Path, ) -> Result< ( - SourceFileName, - Vec<(IsoLiteralExtractionResult, TextSource)>, + RelativePathToSourceFile, + Vec<(IsoLiteralExtractionResult, RelativeTextSource)>, ), Vec>, > { @@ -180,9 +182,10 @@ pub(crate) fn process_iso_literals( pub fn process_iso_literal_extraction( iso_literal_extraction: IsoLiteralExtraction<'_>, - file_name: SourceFileName, + file_name: RelativePathToSourceFile, interned_file_path: FilePath, -) -> Result<(IsoLiteralExtractionResult, TextSource), WithLocation> { +) -> Result<(IsoLiteralExtractionResult, RelativeTextSource), WithLocation> +{ let IsoLiteralExtraction { iso_literal_text, iso_literal_start_index, @@ -190,7 +193,7 @@ pub fn process_iso_literal_extraction( const_export_name, iso_function_called_with_paren: has_paren, } = iso_literal_extraction; - let text_source = TextSource { + let text_source = RelativeTextSource { path: file_name, span: Some(Span::new( iso_literal_start_index as u32, diff --git a/crates/isograph_compiler/src/source_files.rs b/crates/isograph_compiler/src/source_files.rs index f96f9b95c..c846a1828 100644 --- a/crates/isograph_compiler/src/source_files.rs +++ b/crates/isograph_compiler/src/source_files.rs @@ -4,7 +4,7 @@ use std::{ path::{Path, PathBuf}, }; -use common_lang_types::{SourceFileName, TextSource}; +use common_lang_types::{RelativePathToSourceFile, RelativeTextSource}; use graphql_lang_types::{GraphQLTypeSystemDocument, GraphQLTypeSystemExtensionDocument}; use graphql_schema_parser::{parse_schema, parse_schema_extensions}; use intern::string_key::Intern; @@ -25,7 +25,7 @@ use crate::{ #[derive(Clone, Debug, Eq, PartialEq, Default)] pub struct SourceFiles { pub schema: GraphQLTypeSystemDocument, - pub schema_extensions: HashMap, + pub schema_extensions: HashMap, pub contains_iso: ContainsIso, } @@ -262,7 +262,7 @@ fn read_and_parse_graphql_schema( schema_path: &PathBuf, ) -> Result { let content = read_schema_file(schema_path)?; - let schema_text_source = TextSource { + let schema_text_source = RelativeTextSource { path: schema_path .to_str() .expect("Expected schema to be valid string") @@ -275,20 +275,20 @@ fn read_and_parse_graphql_schema( Ok(schema) } -fn intern_file_path(path: &Path) -> SourceFileName { +fn intern_file_path(path: &Path) -> RelativePathToSourceFile { path.to_string_lossy().into_owned().intern().into() } pub fn read_and_parse_schema_extensions( schema_extension_path: &PathBuf, -) -> Result<(SourceFileName, GraphQLTypeSystemExtensionDocument), BatchCompileError> { +) -> Result<(RelativePathToSourceFile, GraphQLTypeSystemExtensionDocument), BatchCompileError> { let file_path = schema_extension_path .to_str() .expect("Expected schema extension to be valid string") .intern() .into(); let extension_content = read_schema_file(schema_extension_path)?; - let extension_text_source = TextSource { + let extension_text_source = RelativeTextSource { path: file_path, span: None, }; @@ -322,7 +322,9 @@ fn process_exposed_fields(schema: &mut UnvalidatedSchema) -> Result<(), BatchCom } #[derive(Clone, Debug, Eq, PartialEq, Default)] -pub struct ContainsIso(pub HashMap>); +pub struct ContainsIso( + pub HashMap>, +); impl ContainsIso { pub fn stats(&self) -> ContainsIsoStats { @@ -351,7 +353,8 @@ impl ContainsIso { } impl Deref for ContainsIso { - type Target = HashMap>; + type Target = + HashMap>; fn deref(&self) -> &Self::Target { &self.0 diff --git a/crates/isograph_lang_parser/src/parse_iso_literal.rs b/crates/isograph_lang_parser/src/parse_iso_literal.rs index db94d1923..7df22b630 100644 --- a/crates/isograph_lang_parser/src/parse_iso_literal.rs +++ b/crates/isograph_lang_parser/src/parse_iso_literal.rs @@ -1,7 +1,7 @@ use std::{collections::HashSet, ops::ControlFlow}; use common_lang_types::{ - FilePath, Location, ScalarFieldName, Span, TextSource, UnvalidatedTypeName, WithLocation, + FilePath, Location, ScalarFieldName, Span, RelativeTextSource, UnvalidatedTypeName, WithLocation, WithSpan, }; use graphql_lang_types::{ @@ -32,7 +32,7 @@ pub fn parse_iso_literal( iso_literal_text: &str, definition_file_path: FilePath, const_export_name: Option<&str>, - text_source: TextSource, + text_source: RelativeTextSource, ) -> Result> { let mut tokens = PeekableLexer::new(iso_literal_text); let discriminator = tokens @@ -70,7 +70,7 @@ pub fn parse_iso_literal( fn parse_iso_entrypoint_declaration( tokens: &mut PeekableLexer<'_>, - text_source: TextSource, + text_source: RelativeTextSource, entrypoint_keyword: Span, ) -> ParseResultWithLocation> { let entrypoint_declaration = tokens @@ -108,7 +108,7 @@ fn parse_iso_client_field_declaration( tokens: &mut PeekableLexer<'_>, definition_file_path: FilePath, const_export_name: Option<&str>, - text_source: TextSource, + text_source: RelativeTextSource, field_keyword_span: Span, ) -> ParseResultWithLocation> { let client_field_declaration = parse_client_field_declaration_inner( @@ -134,7 +134,7 @@ fn parse_client_field_declaration_inner( tokens: &mut PeekableLexer<'_>, definition_file_path: FilePath, const_export_name: Option<&str>, - text_source: TextSource, + text_source: RelativeTextSource, field_keyword_span: Span, ) -> ParseResultWithSpan> { tokens.with_span(|tokens| { @@ -192,7 +192,7 @@ fn parse_iso_client_pointer_declaration( tokens: &mut PeekableLexer<'_>, definition_file_path: FilePath, const_export_name: Option<&str>, - text_source: TextSource, + text_source: RelativeTextSource, field_keyword_span: Span, ) -> ParseResultWithLocation>> { let client_pointer_declaration = parse_client_pointer_declaration_inner( @@ -218,7 +218,7 @@ fn parse_client_pointer_declaration_inner( tokens: &mut PeekableLexer<'_>, definition_file_path: FilePath, const_export_name: Option<&str>, - text_source: TextSource, + text_source: RelativeTextSource, pointer_keyword_span: Span, ) -> ParseResultWithSpan>> { tokens.with_span(|tokens| { @@ -269,7 +269,7 @@ fn parse_client_pointer_declaration_inner( #[allow(clippy::type_complexity)] fn parse_selection_set( tokens: &mut PeekableLexer<'_>, - text_source: TextSource, + text_source: RelativeTextSource, ) -> ParseResultWithSpan>> { let selection_set = parse_optional_selection_set(tokens, text_source)?; match selection_set { @@ -284,7 +284,7 @@ fn parse_selection_set( // TODO this should not parse an optional selection set, but a required one fn parse_optional_selection_set( tokens: &mut PeekableLexer<'_>, - text_source: TextSource, + text_source: RelativeTextSource, ) -> ParseResultWithSpan>>> { let open_brace = tokens.parse_token_of_kind(IsographLangTokenKind::OpenBrace); if open_brace.is_err() { @@ -375,7 +375,7 @@ fn parse_comma_line_break_or_curly(tokens: &mut PeekableLexer<'_>) -> ParseResul fn parse_selection( tokens: &mut PeekableLexer<'_>, - text_source: TextSource, + text_source: RelativeTextSource, ) -> ParseResultWithSpan> { tokens.with_span(|tokens| { let (field_name, alias) = parse_optional_alias_and_field_name(tokens)?; @@ -435,7 +435,7 @@ fn parse_optional_alias_and_field_name( fn parse_directives( tokens: &mut PeekableLexer, - text_source: TextSource, + text_source: RelativeTextSource, ) -> ParseResultWithSpan>> { let mut directives = vec![]; while let Ok(token) = tokens.parse_token_of_kind(IsographLangTokenKind::At) { @@ -456,7 +456,7 @@ fn parse_directives( fn parse_optional_arguments( tokens: &mut PeekableLexer, - text_source: TextSource, + text_source: RelativeTextSource, ) -> ParseResultWithSpan>> { if tokens .parse_token_of_kind(IsographLangTokenKind::OpenParen) @@ -477,7 +477,7 @@ fn parse_optional_arguments( fn parse_argument( tokens: &mut PeekableLexer<'_>, - text_source: TextSource, + text_source: RelativeTextSource, ) -> ParseResultWithSpan> { let argument = tokens.with_span(|tokens| { let name = tokens @@ -540,7 +540,7 @@ fn parse_non_constant_value( fn parse_variable_definitions( tokens: &mut PeekableLexer, - text_source: TextSource, + text_source: RelativeTextSource, ) -> ParseResultWithSpan>>> { if tokens .parse_token_of_kind(IsographLangTokenKind::OpenParen) @@ -561,7 +561,7 @@ fn parse_variable_definitions( fn parse_variable_definition( tokens: &mut PeekableLexer<'_>, - text_source: TextSource, + text_source: RelativeTextSource, ) -> ParseResultWithSpan>> { let variable_definition = tokens.with_span(|tokens| { let _dollar = tokens @@ -589,7 +589,7 @@ fn parse_variable_definition( fn parse_optional_default_value( tokens: &mut PeekableLexer<'_>, - text_source: TextSource, + text_source: RelativeTextSource, ) -> ParseResultWithSpan>> { if tokens .parse_token_of_kind(IsographLangTokenKind::Equals) diff --git a/crates/isograph_lsp/src/semantic_tokens/mod.rs b/crates/isograph_lsp/src/semantic_tokens/mod.rs index 7cc333437..a9b09015a 100644 --- a/crates/isograph_lsp/src/semantic_tokens/mod.rs +++ b/crates/isograph_lsp/src/semantic_tokens/mod.rs @@ -9,7 +9,7 @@ use crate::{ row_col_offset::{diff_to_end_of_slice, get_index_from_diff, RowColDiff}, }; use client_field::client_field_declaration_to_tokens; -use common_lang_types::{Span, TextSource}; +use common_lang_types::{Span, RelativeTextSource}; use entrypoint::entrypoint_declaration_to_tokens; use intern::string_key::Intern; use isograph_compiler::{extract_iso_literals_from_file_content, IsoLiteralExtraction}; @@ -56,7 +56,7 @@ pub fn on_semantic_token_full_request( diff_to_end_of_slice(&file_text[index_of_last_token..iso_literal_start_index]); let file_path = text_document.uri.path().intern(); - let text_source = TextSource { + let text_source = RelativeTextSource { path: file_path.into(), span: Some(Span::new( iso_literal_start_index as u32, diff --git a/crates/isograph_schema/src/process_client_field_declaration.rs b/crates/isograph_schema/src/process_client_field_declaration.rs index f75ed1214..c5595f8ce 100644 --- a/crates/isograph_schema/src/process_client_field_declaration.rs +++ b/crates/isograph_schema/src/process_client_field_declaration.rs @@ -1,7 +1,7 @@ use common_lang_types::{ ConstExportName, FilePath, IsographDirectiveName, IsographObjectTypeName, LinkedFieldName, - Location, ScalarFieldName, SelectableFieldName, TextSource, UnvalidatedTypeName, WithLocation, - WithSpan, + Location, RelativeTextSource, ScalarFieldName, SelectableFieldName, UnvalidatedTypeName, + WithLocation, WithSpan, }; use intern::string_key::Intern; use isograph_lang_types::{ @@ -21,7 +21,7 @@ impl UnvalidatedSchema { pub fn process_client_field_declaration( &mut self, client_field_declaration: WithSpan, - text_source: TextSource, + text_source: RelativeTextSource, ) -> Result<(), WithLocation> { let parent_type_id = self .server_field_data diff --git a/crates/isograph_schema/src/unvalidated_schema.rs b/crates/isograph_schema/src/unvalidated_schema.rs index a21213ee8..f71241ea9 100644 --- a/crates/isograph_schema/src/unvalidated_schema.rs +++ b/crates/isograph_schema/src/unvalidated_schema.rs @@ -1,7 +1,7 @@ use std::collections::{BTreeMap, HashMap}; use common_lang_types::{ - IsographObjectTypeName, JavascriptName, Location, TextSource, UnvalidatedTypeName, + IsographObjectTypeName, JavascriptName, Location, RelativeTextSource, UnvalidatedTypeName, WithLocation, WithSpan, }; use graphql_lang_types::GraphQLTypeAnnotation; @@ -32,7 +32,7 @@ impl SchemaValidationState for UnvalidatedSchemaState { type ClientFieldSelectionScalarFieldAssociatedData = IsographSelectionVariant; type ClientFieldSelectionLinkedFieldAssociatedData = IsographSelectionVariant; type VariableDefinitionInnerType = UnvalidatedTypeName; - type Entrypoint = Vec<(TextSource, WithSpan)>; + type Entrypoint = Vec<(RelativeTextSource, WithSpan)>; } #[derive(Debug, Clone)] diff --git a/crates/isograph_schema/src/validate_entrypoint.rs b/crates/isograph_schema/src/validate_entrypoint.rs index a1f7819d9..37b96bf06 100644 --- a/crates/isograph_schema/src/validate_entrypoint.rs +++ b/crates/isograph_schema/src/validate_entrypoint.rs @@ -1,5 +1,5 @@ use common_lang_types::{ - IsographObjectTypeName, Location, ScalarFieldName, TextSource, UnvalidatedTypeName, + IsographObjectTypeName, Location, ScalarFieldName, RelativeTextSource, UnvalidatedTypeName, WithLocation, WithSpan, }; use isograph_lang_types::{ @@ -12,7 +12,7 @@ use crate::{ClientType, FieldType, UnvalidatedSchema}; impl UnvalidatedSchema { pub fn validate_entrypoint_type_and_field( &self, - text_source: TextSource, + text_source: RelativeTextSource, entrypoint_type_and_field: WithSpan, ) -> Result> { let parent_object_id = self @@ -29,7 +29,7 @@ impl UnvalidatedSchema { fn validate_parent_object_id( &self, parent_type: WithSpan, - text_source: TextSource, + text_source: RelativeTextSource, ) -> Result> { let parent_type_id = self .server_field_data @@ -79,7 +79,7 @@ impl UnvalidatedSchema { fn validate_client_field( &self, field_name: WithSpan, - text_source: TextSource, + text_source: RelativeTextSource, parent_object_id: ServerObjectId, ) -> Result> { let parent_object = self.server_field_data.object(parent_object_id); diff --git a/crates/tests/tests/directives_deserialization.rs b/crates/tests/tests/directives_deserialization.rs index 4b5be5daf..db7e796ce 100644 --- a/crates/tests/tests/directives_deserialization.rs +++ b/crates/tests/tests/directives_deserialization.rs @@ -1,4 +1,4 @@ -use common_lang_types::{SelectableFieldName, StringLiteralValue, TextSource}; +use common_lang_types::{SelectableFieldName, StringLiteralValue, RelativeTextSource}; use graphql_lang_types::{ from_graph_ql_directive, DeserializationError, GraphQLConstantValue, GraphQLDirective, }; @@ -19,7 +19,7 @@ fn unwrap_directive( } fn parse_mutation(source: &str) -> Result, Box> { - let text_source = TextSource { + let text_source = RelativeTextSource { path: "dummy".intern().into(), span: None, }; From 760b2c1a3d2a88b5507425e789ba9df6b178ac77 Mon Sep 17 00:00:00 2001 From: Robert Balicki Date: Sat, 4 Jan 2025 01:50:39 +0900 Subject: [PATCH 06/18] rename more structs to include Embedded in their name --- crates/common_lang_types/src/location.rs | 47 ++++++++++++---------- crates/common_lang_types/src/span.rs | 15 +++++-- crates/graphql_lang_types/src/directive.rs | 4 +- 3 files changed, 39 insertions(+), 27 deletions(-) diff --git a/crates/common_lang_types/src/location.rs b/crates/common_lang_types/src/location.rs index 1ee27d812..b4a467143 100644 --- a/crates/common_lang_types/src/location.rs +++ b/crates/common_lang_types/src/location.rs @@ -33,14 +33,14 @@ impl RelativeTextSource { } #[derive(Copy, Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Hash)] -pub struct EmbeddedLocation { +pub struct EmbeddedRelativeLocation { pub text_source: RelativeTextSource, /// The span is relative to the Source's span, not to the /// entire source file. pub span: Span, } -impl std::fmt::Display for EmbeddedLocation { +impl std::fmt::Display for EmbeddedRelativeLocation { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { let (file_path, read_out_text) = self.text_source.read_to_string(); let text_with_carats = text_with_carats(&read_out_text, self.span); @@ -49,15 +49,15 @@ impl std::fmt::Display for EmbeddedLocation { } } -impl From for Location { - fn from(value: EmbeddedLocation) -> Self { +impl From for Location { + fn from(value: EmbeddedRelativeLocation) -> Self { Location::Embedded(value) } } #[derive(Copy, Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Hash)] pub enum Location { - Embedded(EmbeddedLocation), + Embedded(EmbeddedRelativeLocation), Generated, } @@ -66,7 +66,7 @@ impl Location { Location::Generated } pub fn new(text_source: RelativeTextSource, span: Span) -> Self { - Location::Embedded(EmbeddedLocation::new(text_source, span)) + Location::Embedded(EmbeddedRelativeLocation::new(text_source, span)) } pub fn span(&self) -> Option { @@ -76,9 +76,9 @@ impl Location { } } } -impl EmbeddedLocation { +impl EmbeddedRelativeLocation { pub fn new(text_source: RelativeTextSource, span: Span) -> Self { - EmbeddedLocation { text_source, span } + EmbeddedRelativeLocation { text_source, span } } } @@ -131,7 +131,7 @@ impl WithLocation { /// EmbeddedLocation or WithEmbeddedLocation pub fn hack_to_with_span(self) -> WithSpan { let span = match self.location { - Location::Embedded(EmbeddedLocation { span, .. }) => span, + Location::Embedded(EmbeddedRelativeLocation { span, .. }) => span, Location::Generated => Span::todo_generated(), }; WithSpan { @@ -142,38 +142,41 @@ impl WithLocation { } #[derive(Copy, Clone, Debug, Hash, PartialEq, Eq, Ord, PartialOrd)] -pub struct WithEmbeddedLocation { - pub location: EmbeddedLocation, +pub struct WithEmbeddedRelativeLocation { + pub location: EmbeddedRelativeLocation, pub item: T, } -impl Error for WithEmbeddedLocation { +impl Error for WithEmbeddedRelativeLocation { fn description(&self) -> &str { #[allow(deprecated)] self.item.description() } } -impl fmt::Display for WithEmbeddedLocation { +impl fmt::Display for WithEmbeddedRelativeLocation { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "{}\n{}", self.item, self.location) } } -impl WithEmbeddedLocation { - pub fn new(item: T, location: EmbeddedLocation) -> Self { - WithEmbeddedLocation { item, location } +impl WithEmbeddedRelativeLocation { + pub fn new(item: T, location: EmbeddedRelativeLocation) -> Self { + WithEmbeddedRelativeLocation { item, location } } - pub fn map(self, map: impl FnOnce(T) -> U) -> WithEmbeddedLocation { - WithEmbeddedLocation::new(map(self.item), self.location) + pub fn map(self, map: impl FnOnce(T) -> U) -> WithEmbeddedRelativeLocation { + WithEmbeddedRelativeLocation::new(map(self.item), self.location) } pub fn and_then( self, map: impl FnOnce(T) -> Result, - ) -> Result, E> { - Ok(WithEmbeddedLocation::new(map(self.item)?, self.location)) + ) -> Result, E> { + Ok(WithEmbeddedRelativeLocation::new( + map(self.item)?, + self.location, + )) } pub fn into_with_location(self) -> WithLocation { @@ -181,8 +184,8 @@ impl WithEmbeddedLocation { } } -impl From> for WithLocation { - fn from(value: WithEmbeddedLocation) -> Self { +impl From> for WithLocation { + fn from(value: WithEmbeddedRelativeLocation) -> Self { WithLocation { location: Location::Embedded(value.location), item: value.item, diff --git a/crates/common_lang_types/src/span.rs b/crates/common_lang_types/src/span.rs index d6dc2f555..a67a0f4e1 100644 --- a/crates/common_lang_types/src/span.rs +++ b/crates/common_lang_types/src/span.rs @@ -1,6 +1,9 @@ use std::{fmt, ops::Range}; -use crate::{EmbeddedLocation, Location, RelativeTextSource, WithEmbeddedLocation, WithLocation}; +use crate::{ + EmbeddedRelativeLocation, Location, RelativeTextSource, WithEmbeddedRelativeLocation, + WithLocation, +}; // Invariant: end >= start #[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash, Debug)] @@ -105,8 +108,14 @@ impl WithSpan { WithLocation::new(self.item, Location::new(text_source, self.span)) } - pub fn to_with_embedded_location(self, text_source: RelativeTextSource) -> WithEmbeddedLocation { - WithEmbeddedLocation::new(self.item, EmbeddedLocation::new(text_source, self.span)) + pub fn to_with_embedded_location( + self, + text_source: RelativeTextSource, + ) -> WithEmbeddedRelativeLocation { + WithEmbeddedRelativeLocation::new( + self.item, + EmbeddedRelativeLocation::new(text_source, self.span), + ) } } diff --git a/crates/graphql_lang_types/src/directive.rs b/crates/graphql_lang_types/src/directive.rs index 378d2dcc0..30857d9d4 100644 --- a/crates/graphql_lang_types/src/directive.rs +++ b/crates/graphql_lang_types/src/directive.rs @@ -2,7 +2,7 @@ use std::fmt; use super::{write::write_arguments, NameValuePair}; use crate::GraphQLConstantValue; -use common_lang_types::{DirectiveArgumentName, DirectiveName, WithEmbeddedLocation}; +use common_lang_types::{DirectiveArgumentName, DirectiveName, WithEmbeddedRelativeLocation}; use intern::Lookup; use serde::{ de::{self, value::SeqDeserializer, IntoDeserializer, MapAccess}, @@ -13,7 +13,7 @@ use thiserror::Error; // TODO maybe this should be NameAndArguments and a field should be the same thing...? #[derive(Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Hash)] pub struct GraphQLDirective { - pub name: WithEmbeddedLocation, + pub name: WithEmbeddedRelativeLocation, pub arguments: Vec>, } From 1169c8678ab812835798212f92fc37764cb93c28 Mon Sep 17 00:00:00 2001 From: Robert Balicki Date: Sat, 4 Jan 2025 01:57:08 +0900 Subject: [PATCH 07/18] Add AbsoluteEmbeddedLocation wrapper. Note that this currently doesn't match the description. But this is in the interest of breaking up the commit into smaller chunks. Future commits will align the behavior and the description. --- crates/common_lang_types/src/location.rs | 37 +++++++++++++++---- .../common_lang_types/src/string_key_types.rs | 1 + 2 files changed, 30 insertions(+), 8 deletions(-) diff --git a/crates/common_lang_types/src/location.rs b/crates/common_lang_types/src/location.rs index b4a467143..333f913f2 100644 --- a/crates/common_lang_types/src/location.rs +++ b/crates/common_lang_types/src/location.rs @@ -4,9 +4,9 @@ use intern::Lookup; use crate::{text_with_carats::text_with_carats, RelativePathToSourceFile, Span, WithSpan}; -/// A source, which consists of a filename, and an optional span -/// indicating the subset of the file which corresponds to the -/// source. +/// A source, which consists of a path from the config's project root +/// to the source file, and an optional span indicating the subset of +/// the file which corresponds to the source. /// /// TODO consider whether to replace the span with an index, /// as this will probably mean that sources are more reusable @@ -40,10 +40,23 @@ pub struct EmbeddedRelativeLocation { pub span: Span, } -impl std::fmt::Display for EmbeddedRelativeLocation { +/// What is happening here? EmbeddedRelativeLocation's only contain +/// a path that is relative to the Isograph config's project_root +/// field. Displaying the EmbeddedRelativeLocation involves reading the +/// file and displaying a subset of it. +/// +/// The AbsoluteEmbeddedLocation struct knows how to turn that relative +/// path to an absolute path (i.e. it contains the absolute path to +/// the project_root) for use when reading the file. +#[derive(Copy, Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Hash)] +pub struct AbsoluteEmbeddedLocation { + pub embedded_location: EmbeddedRelativeLocation, +} + +impl std::fmt::Display for AbsoluteEmbeddedLocation { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - let (file_path, read_out_text) = self.text_source.read_to_string(); - let text_with_carats = text_with_carats(&read_out_text, self.span); + let (file_path, read_out_text) = self.embedded_location.text_source.read_to_string(); + let text_with_carats = text_with_carats(&read_out_text, self.embedded_location.span); write!(f, "{}\n{}", file_path, text_with_carats) } @@ -85,7 +98,12 @@ impl EmbeddedRelativeLocation { impl fmt::Display for Location { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { - Location::Embedded(e) => e.fmt(f), + Location::Embedded(embedded_location) => { + let wrapper = AbsoluteEmbeddedLocation { + embedded_location: *embedded_location, + }; + wrapper.fmt(f) + } Location::Generated => { write!(f, "") } @@ -156,7 +174,10 @@ impl Error for WithEmbeddedRelativeLocation { impl fmt::Display for WithEmbeddedRelativeLocation { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "{}\n{}", self.item, self.location) + let wrapper = AbsoluteEmbeddedLocation { + embedded_location: self.location, + }; + write!(f, "{}\n{}", self.item, wrapper) } } diff --git a/crates/common_lang_types/src/string_key_types.rs b/crates/common_lang_types/src/string_key_types.rs index 45c932106..a57a3c4c1 100644 --- a/crates/common_lang_types/src/string_key_types.rs +++ b/crates/common_lang_types/src/string_key_types.rs @@ -111,6 +111,7 @@ string_key_newtype!(IsographDirectiveName); string_key_newtype!(FieldArgumentName); +// This path is from the config's project root to the source file. string_key_newtype!(RelativePathToSourceFile); string_key_newtype!(ArtifactFileType); From bdd06db292f6727ca00cba376e5436937acc6a96 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20Wa=C5=82ach?= <35966385+PatrykWalach@users.noreply.github.com> Date: Fri, 3 Jan 2025 18:42:13 +0100 Subject: [PATCH 08/18] add comment for guards added in `useEffects` --- .../src/useCachedResponsivePrecommitValue.ts | 4 ++++ .../src/useLazyDisposableState.ts | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts b/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts index ee5460a5f..21c2c9c9c 100644 --- a/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts +++ b/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts @@ -51,6 +51,10 @@ export function useCachedResponsivePrecommitValue( const lastCommittedParentCache = useRef | null>(null); useEffect(() => { + // react reruns all `useEffect` in HMR since it doesn't know if the + // code inside of useEffect has changed. Since this is a library + // user can't change this code so we are safe to skip this rerun. + // This also prevents `useEffect` from running twice in Strict Mode. if (lastCommittedParentCache.current === parentCache) { return; } diff --git a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts index 42a857e65..930752c36 100644 --- a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts +++ b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts @@ -32,6 +32,10 @@ export function useLazyDisposableState( const lastCommittedParentCache = useRef | null>(null); useEffect(() => { + // react reruns all `useEffect` in HMR since it doesn't know if the + // code inside of useEffect has changed. Since this is a library + // user can't change this code so we are safe to skip this rerun. + // This also prevents `useEffect` from running twice in Strict Mode. if (lastCommittedParentCache.current === parentCache) { return; } From e1a897d5ed69bb45f2420d6304757fbacd7277db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20Wa=C5=82ach?= <35966385+PatrykWalach@users.noreply.github.com> Date: Tue, 12 Nov 2024 20:50:29 +0100 Subject: [PATCH 09/18] fix error with hot module replacement --- .../src/useCachedResponsivePrecommitValue.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts b/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts index cbe3c86f2..ee5460a5f 100644 --- a/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts +++ b/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts @@ -51,6 +51,10 @@ export function useCachedResponsivePrecommitValue( const lastCommittedParentCache = useRef | null>(null); useEffect(() => { + if (lastCommittedParentCache.current === parentCache) { + return; + } + lastCommittedParentCache.current = parentCache; // On commit, cacheItem may be disposed, because during the render phase, // we only temporarily retained the item, and the temporary retain could have From ad1282aa58c905099b04702f69dc0b058f8eb1fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20Wa=C5=82ach?= <35966385+PatrykWalach@users.noreply.github.com> Date: Tue, 12 Nov 2024 21:18:15 +0100 Subject: [PATCH 10/18] update tests --- libs/isograph-react-disposable-state/package.json | 1 + .../src/useCachedResponsivePrecommitValue.test.tsx | 7 ++++--- pnpm-lock.yaml | 10 ++++++++++ 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/libs/isograph-react-disposable-state/package.json b/libs/isograph-react-disposable-state/package.json index 633ce0083..06f831afe 100644 --- a/libs/isograph-react-disposable-state/package.json +++ b/libs/isograph-react-disposable-state/package.json @@ -25,6 +25,7 @@ }, "devDependencies": { "@types/react": "18.3.1", + "@types/react-test-renderer": "^18.3.0", "react-test-renderer": "^18.2.0", "typescript": "5.6.3" }, diff --git a/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.test.tsx b/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.test.tsx index f743da216..d8415b931 100644 --- a/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.test.tsx +++ b/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.test.tsx @@ -2,7 +2,7 @@ import { describe, test, vi, expect, assert } from 'vitest'; import { ParentCache } from './ParentCache'; import { ItemCleanupPair } from '@isograph/disposable-types'; import { useCachedResponsivePrecommitValue } from './useCachedResponsivePrecommitValue'; -import React from 'react'; +import React, { StrictMode, type ReactElement } from 'react'; import { create } from 'react-test-renderer'; import { CacheItem, CacheItemState } from './CacheItem'; @@ -51,9 +51,10 @@ function promiseAndResolver() { // The fact that sometimes we need to render in concurrent mode and sometimes // not is a bit worrisome. -async function awaitableCreate(Component, isConcurrent) { +async function awaitableCreate(Component: ReactElement, isConcurrent) { const element = create( - Component, + {Component}, + // @ts-expect-error isConcurrent ? { unstable_isConcurrent: true } : undefined, ); await shortPromise(); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 21a8545f2..0cb26c848 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -367,6 +367,9 @@ importers: '@types/react': specifier: 18.3.1 version: 18.3.1 + '@types/react-test-renderer': + specifier: ^18.3.0 + version: 18.3.0 react-test-renderer: specifier: ^18.2.0 version: 18.3.1(react@18.3.1) @@ -2270,6 +2273,9 @@ packages: '@types/react-router@5.1.20': resolution: {integrity: sha512-jGjmu/ZqS7FjSH6owMcD5qpq19+1RS9DeVRqfl1FeBMxTDQAGwlMWOcs52NDoXaNKyG3d1cYQFMs9rCrb88o9Q==} + '@types/react-test-renderer@18.3.0': + resolution: {integrity: sha512-HW4MuEYxfDbOHQsVlY/XtOvNHftCVEPhJF2pQXXwcUiUF+Oyb0usgp48HSgpK5rt8m9KZb22yqOeZm+rrVG8gw==} + '@types/react-transition-group@4.4.11': resolution: {integrity: sha512-RM05tAniPZ5DZPzzNFP+DmrcOdD0efDUxMy3145oljWSl3x9ZV5vhme98gTxFrj2lhXvmGNnUiuDyJgY9IKkNA==} @@ -10317,6 +10323,10 @@ snapshots: '@types/history': 4.7.11 '@types/react': 18.3.1 + '@types/react-test-renderer@18.3.0': + dependencies: + '@types/react': 18.3.1 + '@types/react-transition-group@4.4.11': dependencies: '@types/react': 18.3.1 From 5bc585f4a5c2c0177c2a634bcfba86faf166ad10 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20Wa=C5=82ach?= <35966385+PatrykWalach@users.noreply.github.com> Date: Thu, 14 Nov 2024 15:07:00 +0100 Subject: [PATCH 11/18] fix cleanup --- .../src/useDisposableState.ts | 24 ++++++++++++------- .../src/useLazyDisposableState.test.tsx | 9 +++++-- .../src/useLazyDisposableState.ts | 14 +++++++---- .../src/useUpdatableDisposableState.test.tsx | 5 ++-- 4 files changed, 36 insertions(+), 16 deletions(-) diff --git a/libs/isograph-react-disposable-state/src/useDisposableState.ts b/libs/isograph-react-disposable-state/src/useDisposableState.ts index 46a63fe81..81e18fffe 100644 --- a/libs/isograph-react-disposable-state/src/useDisposableState.ts +++ b/libs/isograph-react-disposable-state/src/useDisposableState.ts @@ -22,7 +22,6 @@ export function useDisposableState( const preCommitItem = useCachedResponsivePrecommitValue( parentCache, (pair) => { - itemCleanupPairRef.current?.[1](); itemCleanupPairRef.current = pair; }, ); @@ -47,14 +46,23 @@ export function useDisposableState( [stateFromDisposableStateHook], ); - useEffect(function cleanupItemCleanupPairRefIfSetStateNotCalled() { - return () => { - if (itemCleanupPairRef.current !== null) { - itemCleanupPairRef.current[1](); - itemCleanupPairRef.current = null; + const lastCommittedParentCache = useRef | null>(null); + + useEffect( + function cleanupItemCleanupPairRefIfSetStateNotCalled() { + if (lastCommittedParentCache.current === parentCache) { + return; } - }; - }, []); + lastCommittedParentCache.current = parentCache; + // capture last set pair in a variable + const current = itemCleanupPairRef.current; + return () => { + // current is a stale variable + current?.[1](); + }; + }, + [parentCache], + ); // Safety: we can be in one of three states. Pre-commit, in which case // preCommitItem is assigned, post-commit but before setState has been diff --git a/libs/isograph-react-disposable-state/src/useLazyDisposableState.test.tsx b/libs/isograph-react-disposable-state/src/useLazyDisposableState.test.tsx index 9d206cd63..38f4c3679 100644 --- a/libs/isograph-react-disposable-state/src/useLazyDisposableState.test.tsx +++ b/libs/isograph-react-disposable-state/src/useLazyDisposableState.test.tsx @@ -1,5 +1,5 @@ import { ItemCleanupPair } from '@isograph/disposable-types'; -import React, { useEffect, useState } from 'react'; +import React, { StrictMode, useEffect, useState } from 'react'; import { create } from 'react-test-renderer'; import { describe, expect, test, vi } from 'vitest'; import { ParentCache } from './ParentCache'; @@ -55,7 +55,12 @@ describe('useLazyDisposableState', async () => { return null; } - const root = create(, { unstable_isConcurrent: true }); + const root = create( + + + , + { unstable_isConcurrent: true }, + ); await committed.promise; expect(cache1.disposeItem).toHaveBeenCalled(); expect(cache1.cache.factory).toHaveBeenCalledOnce(); diff --git a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts index 5c251fd32..38da37c4c 100644 --- a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts +++ b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts @@ -23,18 +23,24 @@ export function useLazyDisposableState( state: T; } { const itemCleanupPairRef = useRef | null>(null); - const preCommitItem = useCachedResponsivePrecommitValue( parentCache, (pair) => { - itemCleanupPairRef.current?.[1](); itemCleanupPairRef.current = pair; }, ); + const lastCommittedParentCache = useRef | null>(null); useEffect(() => { + if (lastCommittedParentCache.current === parentCache) { + return; + } + lastCommittedParentCache.current = parentCache; + // capture last set pair in a variable + const current = itemCleanupPairRef.current; return () => { - const cleanupFn = itemCleanupPairRef.current?.[1]; + // current is a stale variable + const cleanupFn = current?.[1]; // TODO confirm useEffect is called in order. if (cleanupFn == null) { throw new Error( @@ -43,7 +49,7 @@ export function useLazyDisposableState( } return cleanupFn(); }; - }, []); + }, [parentCache]); const returnedItem = preCommitItem?.state ?? itemCleanupPairRef.current?.[0]; diff --git a/libs/isograph-react-disposable-state/src/useUpdatableDisposableState.test.tsx b/libs/isograph-react-disposable-state/src/useUpdatableDisposableState.test.tsx index e74c132c8..48459175e 100644 --- a/libs/isograph-react-disposable-state/src/useUpdatableDisposableState.test.tsx +++ b/libs/isograph-react-disposable-state/src/useUpdatableDisposableState.test.tsx @@ -1,5 +1,5 @@ import { describe, test, vi, expect } from 'vitest'; -import React from 'react'; +import React, { StrictMode } from 'react'; import { create } from 'react-test-renderer'; import { useUpdatableDisposableState, @@ -45,7 +45,8 @@ function promiseAndResolver() { // not is a bit worrisome. async function awaitableCreate(Component, isConcurrent) { const element = create( - Component, + {Component}, + isConcurrent ? { unstable_isConcurrent: true } : undefined, ); await shortPromise(); From ef89d4d1b257542dbea9c3990ef143d0ef415aa7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20Wa=C5=82ach?= <35966385+PatrykWalach@users.noreply.github.com> Date: Thu, 14 Nov 2024 15:26:30 +0100 Subject: [PATCH 12/18] don't use stale itemCleanupPairRef --- .../src/useDisposableState.ts | 8 ++++---- .../src/useLazyDisposableState.ts | 6 ++---- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/libs/isograph-react-disposable-state/src/useDisposableState.ts b/libs/isograph-react-disposable-state/src/useDisposableState.ts index 81e18fffe..d5e979459 100644 --- a/libs/isograph-react-disposable-state/src/useDisposableState.ts +++ b/libs/isograph-react-disposable-state/src/useDisposableState.ts @@ -54,11 +54,11 @@ export function useDisposableState( return; } lastCommittedParentCache.current = parentCache; - // capture last set pair in a variable - const current = itemCleanupPairRef.current; + return () => { - // current is a stale variable - current?.[1](); + if (itemCleanupPairRef.current !== null) { + itemCleanupPairRef.current[1](); + } }; }, [parentCache], diff --git a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts index 38da37c4c..42a857e65 100644 --- a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts +++ b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts @@ -36,11 +36,9 @@ export function useLazyDisposableState( return; } lastCommittedParentCache.current = parentCache; - // capture last set pair in a variable - const current = itemCleanupPairRef.current; + return () => { - // current is a stale variable - const cleanupFn = current?.[1]; + const cleanupFn = itemCleanupPairRef.current?.[1]; // TODO confirm useEffect is called in order. if (cleanupFn == null) { throw new Error( From c8d6a768df36cef1dc4ffe167d720ace48c33b50 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20Wa=C5=82ach?= <35966385+PatrykWalach@users.noreply.github.com> Date: Fri, 3 Jan 2025 18:42:13 +0100 Subject: [PATCH 13/18] add comment for guards added in `useEffects` --- .../src/useCachedResponsivePrecommitValue.ts | 4 ++++ .../src/useLazyDisposableState.ts | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts b/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts index ee5460a5f..21c2c9c9c 100644 --- a/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts +++ b/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts @@ -51,6 +51,10 @@ export function useCachedResponsivePrecommitValue( const lastCommittedParentCache = useRef | null>(null); useEffect(() => { + // react reruns all `useEffect` in HMR since it doesn't know if the + // code inside of useEffect has changed. Since this is a library + // user can't change this code so we are safe to skip this rerun. + // This also prevents `useEffect` from running twice in Strict Mode. if (lastCommittedParentCache.current === parentCache) { return; } diff --git a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts index 42a857e65..930752c36 100644 --- a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts +++ b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts @@ -32,6 +32,10 @@ export function useLazyDisposableState( const lastCommittedParentCache = useRef | null>(null); useEffect(() => { + // react reruns all `useEffect` in HMR since it doesn't know if the + // code inside of useEffect has changed. Since this is a library + // user can't change this code so we are safe to skip this rerun. + // This also prevents `useEffect` from running twice in Strict Mode. if (lastCommittedParentCache.current === parentCache) { return; } From b18976db7149316ae01de7a843c8a5946e6374a5 Mon Sep 17 00:00:00 2001 From: Robert Balicki Date: Sat, 4 Jan 2025 03:34:32 +0900 Subject: [PATCH 14/18] run cargo fmt --- crates/graphql_schema_parser/src/parse_schema.rs | 4 ++-- crates/isograph_lang_parser/src/parse_iso_literal.rs | 4 ++-- crates/isograph_lsp/src/semantic_tokens/mod.rs | 2 +- crates/isograph_schema/src/validate_entrypoint.rs | 2 +- crates/tests/tests/directives_deserialization.rs | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/crates/graphql_schema_parser/src/parse_schema.rs b/crates/graphql_schema_parser/src/parse_schema.rs index 75f26d1c8..74bb13a20 100644 --- a/crates/graphql_schema_parser/src/parse_schema.rs +++ b/crates/graphql_schema_parser/src/parse_schema.rs @@ -1,8 +1,8 @@ use std::{ops::ControlFlow, str::FromStr}; use common_lang_types::{ - DescriptionValue, EnumLiteralValue, GraphQLInterfaceTypeName, GraphQLObjectTypeName, Span, - StringLiteralValue, RelativeTextSource, WithLocation, WithSpan, + DescriptionValue, EnumLiteralValue, GraphQLInterfaceTypeName, GraphQLObjectTypeName, + RelativeTextSource, Span, StringLiteralValue, WithLocation, WithSpan, }; use graphql_syntax::TokenKind; use intern::{ diff --git a/crates/isograph_lang_parser/src/parse_iso_literal.rs b/crates/isograph_lang_parser/src/parse_iso_literal.rs index 7df22b630..3ccc7f4fb 100644 --- a/crates/isograph_lang_parser/src/parse_iso_literal.rs +++ b/crates/isograph_lang_parser/src/parse_iso_literal.rs @@ -1,8 +1,8 @@ use std::{collections::HashSet, ops::ControlFlow}; use common_lang_types::{ - FilePath, Location, ScalarFieldName, Span, RelativeTextSource, UnvalidatedTypeName, WithLocation, - WithSpan, + FilePath, Location, RelativeTextSource, ScalarFieldName, Span, UnvalidatedTypeName, + WithLocation, WithSpan, }; use graphql_lang_types::{ GraphQLListTypeAnnotation, GraphQLNamedTypeAnnotation, GraphQLNonNullTypeAnnotation, diff --git a/crates/isograph_lsp/src/semantic_tokens/mod.rs b/crates/isograph_lsp/src/semantic_tokens/mod.rs index a9b09015a..d7fa6ff4c 100644 --- a/crates/isograph_lsp/src/semantic_tokens/mod.rs +++ b/crates/isograph_lsp/src/semantic_tokens/mod.rs @@ -9,7 +9,7 @@ use crate::{ row_col_offset::{diff_to_end_of_slice, get_index_from_diff, RowColDiff}, }; use client_field::client_field_declaration_to_tokens; -use common_lang_types::{Span, RelativeTextSource}; +use common_lang_types::{RelativeTextSource, Span}; use entrypoint::entrypoint_declaration_to_tokens; use intern::string_key::Intern; use isograph_compiler::{extract_iso_literals_from_file_content, IsoLiteralExtraction}; diff --git a/crates/isograph_schema/src/validate_entrypoint.rs b/crates/isograph_schema/src/validate_entrypoint.rs index 37b96bf06..e9a576d9a 100644 --- a/crates/isograph_schema/src/validate_entrypoint.rs +++ b/crates/isograph_schema/src/validate_entrypoint.rs @@ -1,5 +1,5 @@ use common_lang_types::{ - IsographObjectTypeName, Location, ScalarFieldName, RelativeTextSource, UnvalidatedTypeName, + IsographObjectTypeName, Location, RelativeTextSource, ScalarFieldName, UnvalidatedTypeName, WithLocation, WithSpan, }; use isograph_lang_types::{ diff --git a/crates/tests/tests/directives_deserialization.rs b/crates/tests/tests/directives_deserialization.rs index db7e796ce..9dfc1da24 100644 --- a/crates/tests/tests/directives_deserialization.rs +++ b/crates/tests/tests/directives_deserialization.rs @@ -1,4 +1,4 @@ -use common_lang_types::{SelectableFieldName, StringLiteralValue, RelativeTextSource}; +use common_lang_types::{RelativeTextSource, SelectableFieldName, StringLiteralValue}; use graphql_lang_types::{ from_graph_ql_directive, DeserializationError, GraphQLConstantValue, GraphQLDirective, }; From db101241a9a22a5166a3b3c1ab7f8e6dfa561453 Mon Sep 17 00:00:00 2001 From: Robert Balicki Date: Sun, 5 Jan 2025 00:58:29 +0900 Subject: [PATCH 15/18] AbsoluteEmbeddedLocation doesnt need to implement Copy --- crates/common_lang_types/src/location.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/common_lang_types/src/location.rs b/crates/common_lang_types/src/location.rs index 333f913f2..177915843 100644 --- a/crates/common_lang_types/src/location.rs +++ b/crates/common_lang_types/src/location.rs @@ -48,7 +48,7 @@ pub struct EmbeddedRelativeLocation { /// The AbsoluteEmbeddedLocation struct knows how to turn that relative /// path to an absolute path (i.e. it contains the absolute path to /// the project_root) for use when reading the file. -#[derive(Copy, Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Hash)] +#[derive(Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Hash)] pub struct AbsoluteEmbeddedLocation { pub embedded_location: EmbeddedRelativeLocation, } From 8a2ecd05d7234b58dfdf4950aa8b2b08838e3b2e Mon Sep 17 00:00:00 2001 From: Robert Balicki Date: Sun, 5 Jan 2025 01:18:26 +0900 Subject: [PATCH 16/18] Create AbsoluteLocation struct - Follows the logic of the AbsoluteEmbeddedLocation struct, namely that it will require an absolute path to Isograph config's project root for std::fmt::Display --- crates/common_lang_types/src/location.rs | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/crates/common_lang_types/src/location.rs b/crates/common_lang_types/src/location.rs index 177915843..1094cf002 100644 --- a/crates/common_lang_types/src/location.rs +++ b/crates/common_lang_types/src/location.rs @@ -95,13 +95,15 @@ impl EmbeddedRelativeLocation { } } -impl fmt::Display for Location { +struct AbsoluteLocation { + pub location: Location, +} + +impl fmt::Display for AbsoluteLocation { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { + match self.location { Location::Embedded(embedded_location) => { - let wrapper = AbsoluteEmbeddedLocation { - embedded_location: *embedded_location, - }; + let wrapper = AbsoluteEmbeddedLocation { embedded_location }; wrapper.fmt(f) } Location::Generated => { @@ -126,7 +128,10 @@ impl Error for WithLocation { impl fmt::Display for WithLocation { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "{}\n{}", self.item, self.location) + let absolute_location = AbsoluteLocation { + location: self.location, + }; + write!(f, "{}\n{}", self.item, absolute_location) } } From b9ec7bd615d6f11f438c90f94bbd2614d8964cd2 Mon Sep 17 00:00:00 2001 From: Robert Balicki Date: Sun, 5 Jan 2025 01:49:28 +0900 Subject: [PATCH 17/18] std::fmt::Display the directive name directly, not the WithEmbededRelativeLocation --- crates/graphql_lang_types/src/directive.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/graphql_lang_types/src/directive.rs b/crates/graphql_lang_types/src/directive.rs index 30857d9d4..18eb41148 100644 --- a/crates/graphql_lang_types/src/directive.rs +++ b/crates/graphql_lang_types/src/directive.rs @@ -19,7 +19,7 @@ pub struct GraphQLDirective { impl fmt::Display for GraphQLDirective { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "@{}", self.name)?; + write!(f, "@{}", self.name.item)?; write_arguments(f, &self.arguments)?; Ok(()) } From 0a63b51dc42e7ba16e1d0080f84d8691047c87fc Mon Sep 17 00:00:00 2001 From: Robert Balicki Date: Sun, 5 Jan 2025 02:04:23 +0900 Subject: [PATCH 18/18] remove unneeded impls --- crates/common_lang_types/src/location.rs | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/crates/common_lang_types/src/location.rs b/crates/common_lang_types/src/location.rs index 1094cf002..ff38e3248 100644 --- a/crates/common_lang_types/src/location.rs +++ b/crates/common_lang_types/src/location.rs @@ -170,22 +170,6 @@ pub struct WithEmbeddedRelativeLocation { pub item: T, } -impl Error for WithEmbeddedRelativeLocation { - fn description(&self) -> &str { - #[allow(deprecated)] - self.item.description() - } -} - -impl fmt::Display for WithEmbeddedRelativeLocation { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - let wrapper = AbsoluteEmbeddedLocation { - embedded_location: self.location, - }; - write!(f, "{}\n{}", self.item, wrapper) - } -} - impl WithEmbeddedRelativeLocation { pub fn new(item: T, location: EmbeddedRelativeLocation) -> Self { WithEmbeddedRelativeLocation { item, location }