Fix strict lookup in compat mode - #2150
Merged
kibertoad merged 1 commit intoApr 21, 2026
Merged
Conversation
theodorejb
force-pushed
the
fix-compat-strict-lookup
branch
2 times, most recently
from
April 15, 2026 20:02
c5900c7 to
0b0f7b3
Compare
theodorejb
changed the title
theodorejb
force-pushed
the
fix-compat-strict-lookup
branch
2 times, most recently
from
April 15, 2026 22:37
a6dd9a6 to
2f55d99
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes incorrect strict lookups when compat mode is enabled, preventing TypeError crashes and restoring expected strict error behavior.
Changes:
- Added strict+compat regression tests covering boolean block contexts,
eachitem lookups, recursion, and null handling. - Introduced a runtime helper (
container.strictLookup) to safely traversedepthsfor strict property resolution in compat scenarios. - Updated the JS compiler to emit
container.strictLookup(...)when compat mode has already performed a depthed lookup.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| spec/strict.js | Adds test coverage for strict+compat mode behaviors and regressions. |
| lib/handlebars/runtime.js | Adds container.strictLookup to safely resolve strict properties across depths in compat mode. |
| lib/handlebars/compiler/javascript-compiler.js | Emits container.strictLookup(...) in the strict+compat codegen path to avoid in operator crashes on primitives. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
theodorejb
force-pushed
the
fix-compat-strict-lookup
branch
from
April 19, 2026 19:48
2f55d99 to
255ef4c
Compare
Contributor
|
One suggestion: add a direct "{{v}} with strict+compat where v doesn't exist at any depth" test. Your "still throw when a property is missing at all depths" covers the same code path but via {{#each items}}, and a minimal top-level case would more clearly regress-guard the originally reported form. |
theodorejb
force-pushed
the
fix-compat-strict-lookup
branch
from
April 21, 2026 19:16
255ef4c to
cd47c94
Compare
Contributor
Author
|
@kibertoad I rebased now and added a test case for a simple |
kibertoad
approved these changes
Apr 21, 2026
Contributor
|
thank you! |
theodorejb
added a commit
to theodorejb/handlebars.js
that referenced
this pull request
Apr 30, 2026
Follow-up to handlebars-langGH-2150. `container.lookup` used `result != null` which caused explicit null values to fall through to parent contexts. This was inconsistent with expected behavior and with Mustache, which stops and returns a key when present in the object, regardless of its value. The whole point of `compat` mode is to match Mustache's lookup semantics, so I don't think this behavior was intended. There were no tests for `compat` with an explicit null value, apart from a couple `compat` + `strict` mode tests I just added myself in handlebars-langGH-2150. One of these tests ("should still perform recursive lookup with a multi-part path not in context") was inconsistent with the test immediately below it ("should directly return an explicitly null property"), and that is fixed here. This change makes it possible for unify the lookup logic in `container.lookup` and `container.strictLookup`, additionally fixing an issue where `strictLookup` didn't respect the `allowedProtoMethods` runtime option the same as `lookup` (see the added test in `spec/security.js` compared to the test above it).
theodorejb
added a commit
to theodorejb/handlebars.js
that referenced
this pull request
May 17, 2026
Follow-up to handlebars-langGH-2150. `container.lookup` used `result != null` which caused explicit null values to fall through to parent contexts. This was inconsistent with expected behavior and with Mustache, which stops and returns a key when present in the object, regardless of its value. The whole point of `compat` mode is to match Mustache's lookup semantics, so I don't think this behavior was intended. There were no tests for `compat` with an explicit null value, apart from a couple `compat` + `strict` mode tests I just added myself in handlebars-langGH-2150. One of these tests ("should still perform recursive lookup with a multi-part path not in context") was inconsistent with the test immediately below it ("should directly return an explicitly null property"), and that is fixed here. This change makes it possible for unify the lookup logic in `container.lookup` and `container.strictLookup`, additionally fixing an issue where `strictLookup` didn't respect the `allowedProtoMethods` runtime option the same as `lookup` (see the added test in `spec/security.js` compared to the test above it). This also fixes Map value resolution in compat-mode depthed lookup (included test by @TorMatzAndren from handlebars-lang#2156).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2149, fixes #1741