Skip to content

[WIP] Begin moving towards invokable wire format #1690

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 50 commits into
base: main
Choose a base branch
from

Conversation

wycats
Copy link
Contributor

@wycats wycats commented Jan 23, 2025

No description provided.

Copy link
Contributor

github-actions bot commented Jan 23, 2025

This PRmain
Dev
706K └─┬ .
233K   ├── syntax
168K   ├── runtime
133K   ├── compiler
 68K   ├── opcode-compiler
 27K   ├── manager
 24K   ├── validator
 13K   ├── program
8.9K   ├── reference
7.2K   ├── destroyable
6.3K   ├── util
4.3K   ├── node
3.9K   ├── wire-format
3.4K   ├── global-context
1.0K   ├── vm
969B   ├── encoder
844B   ├── vm-babel-plugins
606B   └── owner
588K └─┬ .
169K   ├── runtime
160K   ├── syntax
100K   ├── compiler
 58K   ├── opcode-compiler
 27K   ├── manager
 24K   ├── validator
 11K   ├── program
8.9K   ├── reference
7.2K   ├── destroyable
6.3K   ├── util
4.3K   ├── node
3.4K   ├── global-context
2.5K   ├── wire-format
1.0K   ├── vm
969B   ├── encoder
844B   ├── vm-babel-plugins
606B   └── owner
Prod
286K └─┬ .
103K   ├── syntax
 63K   ├── runtime
 63K   ├── compiler
 21K   ├── opcode-compiler
7.9K   ├── manager
5.6K   ├── program
5.1K   ├── validator
3.6K   ├── reference
2.6K   ├── wire-format
2.4K   ├── util
2.1K   ├── node
1.5K   ├── destroyable
737B   ├── vm
594B   ├── global-context
516B   ├── encoder
469B   ├── vm-babel-plugins
155B   └── owner
231K └─┬ .
 70K   ├── syntax
 63K   ├── runtime
 48K   ├── compiler
 18K   ├── opcode-compiler
7.9K   ├── manager
5.1K   ├── validator
4.8K   ├── program
3.6K   ├── reference
2.4K   ├── util
2.1K   ├── node
1.6K   ├── wire-format
1.5K   ├── destroyable
737B   ├── vm
594B   ├── global-context
516B   ├── encoder
469B   ├── vm-babel-plugins
155B   └── owner

@wycats wycats force-pushed the feature/emit-fn-calls branch 2 times, most recently from cc230f3 to 4625e5e Compare February 6, 2025 07:26
@wycats wycats force-pushed the feature/emit-fn-calls branch from a5f57d9 to cddc4dc Compare February 12, 2025 02:52
@wycats wycats force-pushed the feature/emit-fn-calls branch from 6a5d569 to 87e01ac Compare February 20, 2025 03:18
NullVoxPopuli added a commit that referenced this pull request Feb 20, 2025
NullVoxPopuli added a commit that referenced this pull request Feb 20, 2025
NullVoxPopuli added a commit that referenced this pull request Feb 20, 2025
NullVoxPopuli added a commit that referenced this pull request Feb 20, 2025
NullVoxPopuli added a commit that referenced this pull request Feb 20, 2025
@github-actions github-actions bot mentioned this pull request Feb 20, 2025
@wycats wycats force-pushed the feature/emit-fn-calls branch 3 times, most recently from 0683762 to 9ba08a7 Compare February 23, 2025 08:37
@github-actions github-actions bot mentioned this pull request Mar 4, 2025
@wycats wycats force-pushed the feature/emit-fn-calls branch 2 times, most recently from 2b97bec to ecadfb7 Compare March 19, 2025 01:04
@NullVoxPopuli NullVoxPopuli force-pushed the feature/emit-fn-calls branch from ecadfb7 to ccb6165 Compare March 25, 2025 13:56
@NullVoxPopuli NullVoxPopuli requested a review from Copilot March 26, 2025 21:11
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR begins transitioning towards an invokable wire format by updating integration test harnesses, refining event recording and cache behavior, and adding comprehensive documentation and pseudocode for the Glimmer reactivity system.

  • Updated integration tests with new CSS and improved event recording handling.
  • Refactored build configuration to adjust the typescript helper and updated the benchmark environment to reference layout compilation.
  • Added extensive documentation and pseudocode for the reactive system, including tags, primitives, and composition.

Reviewed Changes

Copilot reviewed 171 out of 176 changed files in this pull request and generated no comments.

Show a summary per file
File Description
packages/@glimmer-workspace/integration-tests/lib/setup-harness.ts Added import for new CSS tweaks.
packages/@glimmer-workspace/integration-tests/lib/render-test.ts Extended test interfaces and recording event functionality.
packages/@glimmer-workspace/integration-tests/lib/modes/jit/delegate.ts Minor whitespace adjustment.
packages/@glimmer-workspace/integration-tests/lib/compile.ts Updated precompileJSON call with meta information.
packages/@glimmer-workspace/integration-tests/index.ts Updated exports to include additional test helpers.
packages/@glimmer-workspace/build/lib/config.js Refactored typescript function signature and adjusted SWC settings.
packages/@glimmer-workspace/benchmark-env/lib/benchmark/util.ts Replaced reference to compilable with layout in entry compilation.
guides/reactivity/* New and updated documentation and pseudocode for the reactivity system.
Files not reviewed (5)
  • .prototools: Language not supported
  • .vscode/settings.json: Language not supported
  • package.json: Language not supported
  • packages/@glimmer-workspace/build/package.json: Language not supported
  • packages/@glimmer-workspace/integration-tests/lib/harness/tweaks.css: Language not supported
Comments suppressed due to low confidence (3)

packages/@glimmer-workspace/build/lib/config.js:410

  • The typescript() function signature was changed by removing the package parameter. Please ensure all callers have been updated accordingly and update any related documentation or JSDoc to reflect this change.
              typescript(env),

packages/@glimmer-workspace/benchmark-env/lib/benchmark/util.ts:5

  • The property reference was changed from 'compilable' to 'layout'. Verify that 'layout' exists on entry and that this change is consistent with the intended API to avoid compilation issues.
  return unwrapHandle(entry.layout!.compile(context));

packages/@glimmer-workspace/integration-tests/lib/render-test.ts:112

  • [nitpick] This diff introduces private class fields (using the '#' syntax). Ensure that the target TypeScript/JavaScript environment fully supports private fields to avoid runtime errors.
  #events: RecordedEvent[] = [];

@reteps
Copy link

reteps commented Apr 10, 2025

Any way that I can contribute? In the context of #1666, this would be very helpful.

@NullVoxPopuli
Copy link
Contributor

idk, basically what's left here is that I need to add tests, and make sure CI goes green

NullVoxPopuli added a commit to NullVoxPopuli/ember.js that referenced this pull request Apr 25, 2025
NullVoxPopuli added a commit to NullVoxPopuli/ember.js that referenced this pull request Apr 25, 2025
wycats and others added 24 commits May 28, 2025 21:49
- Remove @glimmer-workspace/env from types array (workspace packages don't work in types array)
- Fix type errors in @glimmer/program using isIndexable utility
- Update import paths to use public APIs instead of deep imports
- Add proper type guards for unknown types passed to typed functions

These minimal changes resolve the TypeScript compilation errors that were blocking the build. The build now successfully compiles all packages but hits memory limits during production bundling (separate issue).
- Add tools in bin/fixes/ for applying ESLint suggestions and TypeScript code fixes
- Update CLAUDE.md to document automation tools for future reference
- Fix unnecessary escape characters in strict-mode-test.ts
- Tools enable systematic fixing of linting issues for both humans and LLMs
- Remove unused keyword filtering variables in keyword-errors-test.ts
- Remove debugger statement in verify.ts
- Remove unused 'full' variable in syntax-error.ts
- Add future-github-issues.md to track follow-up improvements

Reduces linting errors from 51 to 48
- Fix unbound method warnings by wrapping QUnit methods in arrow functions
- Remove unused TYPES variable in keyword-errors-test.ts
- Remove unused RenderTestFunction interface

Reduces linting errors from 48 to 42
- Fix deep import path in curry-value.ts to use public API
- Remove empty block statement in normalization pass
- Both changes verified with successful build

Reduces linting errors from 42 to 40
- Remove unused KeywordType import
- Remove unused private fields: #append, #named, #span, #container, #parent, #curly
- Prefix unused parameters with underscore: _named, _span
- All removed code was genuinely unused dead code

Reduces linting errors from 33 to 24
- Remove unused private fields from GlimmerSyntaxError class
- Replace non-null assertion with proper error checking in let statement compilation
- Update deprecated blockParams API to use params for better error propagation
- Mark unused constructor parameters with underscore prefix

Reduces linting errors from ~51 to 17, with remaining errors being documented unsafe TypeScript operations.
…rence

The original commit 86a354a "Begin moving towards invokable wire format" was incomplete:
- serialize.ts expected ASTv2.ResolvedVarReference but the class was missing
- isVariableReference() checked for 'Resolved' type but ResolvedVarReference didn't exist
- FreeVarReference was renamed to LexicalVarReference but resolved variant was missing

Changes:
- Add missing ResolvedVarReference class to complete the refactor
- Add AnyArgs union type for serialize functions
- Add trusting property to ComponentArg class
- Update supporting code to handle the restored types

Resolves all 16 TypeScript unsafe operation linting errors.
- Uses existing bin/ infrastructure (chalk, execa)
- Focuses on essential checks (linting) without expensive builds
- Avoids TypeScript node_modules traversal issues
- Provides quick validation before pushing changes
- Fix undefined checks in ESLint suggestion scripts
- Add missing LanguageServiceHost methods for TypeScript codefixes
- Improve error handling in automation scripts
- Update ESLint configuration for executable TypeScript files
- Simplify CI validation script to focus on linting
@wycats wycats force-pushed the feature/emit-fn-calls branch from 6aa74e5 to dd2e3dd Compare May 29, 2025 06:17
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.

3 participants