Upgrade to support dart 3.12 - #84
Open
dukefirehawk wants to merge 106 commits into
Open
Conversation
Both bodies are a single `.toJS`, which yields a non-nullable `JSExportedDartFunction`, so the nullable return type was never reachable. It did not type-check against `EventManager.addEventListener`, which takes a non-nullable `JSFunction` and immediately calls it. Generated code only routes through `EventManager` for events it has to intercept -- the keyed ones such as `keydown.enter`, which contain the `.` delimiter that `KeyEvents.supports` matches -- so this broke exactly those bindings, and only in the generated `.template.dart`, where it cannot be worked around from application code. Plain DOM events go straight to `element.addEventListener` and were unaffected.
`provideTypeOptional` is documented as returning null when no provider is found, but its body was identical to `provideType` -- it called `get(token)` without a not-found value, so it threw instead. 6281ad5 changed the return type from `T?` to `T` and dropped the `null` argument, and removed the matching `if (testability != null)` guard in `ApplicationRef`. `Testability` is only bound when bootstrapping for tests, so the sole caller always takes the not-found path: every application throws "No provider found for Testability" during bootstrap. Restore both halves. The guard is required as well as the return type, since `registerApplication` takes a non-nullable `Testability`.
A `read:` token is matched by identity against what the element publishes, which includes `Element` and `HTMLElement` from `package:web`. Those are extension types, so the erased `toTypeValue()` in `_getQuery` produced `JSObject` from `dart:_interceptors`, which matches nothing. The result was a silently dropped query: the generated view creates the element but never assigns it to the annotated field, so the field stays null and the only symptom is a null dereference at runtime. Missed by 45dba97, which un-erased the provider and token paths but not this one -- it is a query read token rather than a provider token.
When an explicit `read:` token resolved to no provider, `beforeChildren` left the query value null and skipped `addQueryResult`, emitting no query at all. The annotated field then stayed null forever, surfacing only as a null dereference at runtime with nothing in the build output to point at it. Throw a BuildError naming the field and the unresolved token instead, and say which tokens an element actually publishes -- the common mistake is `dart:html` `Element`, which is a different token from `package:web` `Element` despite the identical name. Thrown rather than reported through `CompileContext.reportAndRecover`: `throwRecoverableErrors` is only called from the template parser, so anything reported this late in view compilation is collected and never surfaced. That was verified the hard way -- the first version of this check used `reportAndRecover` and was itself silently swallowed.
Two constructor parameters are declared nullable but not `@Optional()`, which the compiler requires of each other in null-safe libraries: a nullable type means DI is allowed to find nothing, and `@Optional()` is what tells it to return null rather than throw. `NgControlGroup` takes `@SkipSelf() this._parent` against a `ControlContainer? _parent`, and uses it null-tolerantly throughout (`controlPath(name, _parent)`, `_parent?.formDirective`). Its sibling `NgControlName` declares the identical field with `@Optional() @SkipSelf()`. The annotation was simply never added when the field went nullable. `MemorizedForm` forwards `super.changeDetectorRef` to `NgForm(@optional() ChangeDetectorRef? changeDetectorRef)`. A super parameter inherits the declared type but not the annotations, so it reads as nullable and required; the sibling parameter on the same constructor repeats its annotations explicitly for the same reason. Both are inert today, because every positional parameter is currently treated as optional regardless of annotation. They become build errors as soon as that is fixed.
6281ad5 rewrote `ParameterInfo.isPositional` from `_parameter.parameterKind == ParameterKind.POSITIONAL` to `_parameter.isPositional`. Those read as synonyms and are not: `ParameterKind.POSITIONAL` is the *optional* positional kind (`[Foo foo]`), while `isPositional` is true for `ParameterKind.REQUIRED` as well. So `isOptional || isPositional` stopped meaning "@optional(), or declared `[Foo foo]`" and started meaning "@optional(), or any positional parameter" -- i.e. every ordinary injected dependency in the application. The visible effect is that `injectorGet`, which throws a NoProviderError naming the token it could not find, is emitted as `injectorGetOptional`, which returns null. A missing provider then travels as a null into a non-nullable field and surfaces much later as an unrelated-looking cast failure. In one application this turned 8 required lookups and 1674 optional ones; restoring the intended meaning turns it back into 1400 and 290, and the 290 are all genuine `@Optional()` tokens. The checked-in DI goldens still record `injectorGet` for required positional dependencies -- they were never regenerated after the regression -- and match again. Rename the getter to `isOptionalPositional` rather than repoint it, since the old name is exactly what invited the mistake, and drop the stale `deprecated_member_use` ignore: `isOptionalPositional` is not deprecated. The same commit also commented out the "non-nullable must *not* be @optional()" guard in `_checkForOptionalAndNullable` with "TODO: Check why is is not working as expected". It was not working because of the flip three hundred lines below it: with every positional parameter marked optional, the guard fired on essentially every component. Restore it. With the meaning fixed, it flags exactly two real defects across ngdart, ngforms, ngrouter and one large application, both fixed in the preceding commit.
6281ad5 replaced the body of `removeNodes` with `nodes.removeRange(0, nodes.length)` and commented out the loop that detached each node, leaving behind a `nodes.delete()` that never compiled -- `delete` on the List, not on each node. `removeRange` empties the Dart `List<Node>`: it drops Angular's references to the nodes but never touches the document, so nothing is removed from the page. `removeNodes` is the only teardown path under `ViewContainerRef.clear()` -> `remove()` -> `detachView()` -> `removeRootNodes()`, so every construct that destroys an embedded view was affected. `*ngIf` left its content rendered when the condition went false, and successive change-detection passes inserted new views alongside the stale ones instead of replacing them -- one test observed three copies of a node where it expected one. Restore the loop in the JS-interop form 69426d3 had settled on, `node.parentNode?.removeChild(node)`, which also tolerates an already-detached node. Dropping `removeRange` additionally restores the `domRootRendererIsDirty` signal in `EmbeddedView.removeRootNodes`, which tests `rootNodes.isNotEmpty` after the call and so was always false once the list had been cleared. _tests/test/common/directives/if_test.dart goes from 2/5 to 5/5.
The encapsulation optimization checked `templateMeta.styleUrls` instead of `templateMeta.styles`. Since `allExternalStyles` is itself derived from `styleUrls`, both clauses tested the same thing and the condition collapsed to "no external stylesheets" — inline `styles:` stopped counting entirely. Any component with inline styles and no `styleUrls:` was silently downgraded to ViewEncapsulation.none, so the compiler emitted ComponentStyles.unscoped() with unshimmed CSS. No _nghost-/_ngcontent- classes were applied, `:host` never matched, and plain element selectors leaked into child components. Regressed in 6281ad5. Fixes 10 failing tests in _tests/test/core/styling/shim_test.dart (3/13 -> 13/13).
1747422 restored the guard requiring a nullable DI parameter to be annotated @optional(). The guard had been inert because `isOptional` was computed as `isOptional || isPositional`, and `isPositional` is true for required positional parameters too, so every constructor parameter reported itself optional. Eight test components across ngforms, ngrouter and ngtest declare a nullable dependency without the annotation and were only ever accepted because of that. They now fail the build, which is why these three suites have not been running. Seven of the eight are spuriously nullable rather than genuinely optional: the dependency is always provided, either by the component's own `providers:` or as a built-in like ChangeDetectorRef, and the tests say so themselves. override_test awaits `_service!.fetch()`, bootstrap_test asserts `expect(instance._testService, isNotNull)`, and on_navigation_start_test unwraps `assertOnlyInstance.router!` four times. Make those parameters non-nullable and drop the defensive `?.`/`!` at the use sites. Annotating them @optional() would also have satisfied the compiler, but it emits `injectorGetOptional` in place of `injectorGet` -- exactly the silent null 1747422 set out to remove. A missing provider would travel as null into a non-nullable field instead of throwing NoProviderError naming the token. The exception is CustomEditorWithNgModelSupport in ng_model_test, which injects an NgControl into a ControlValueAccessor. An accessor can legitimately exist without a control, and the constructor body already guards with `_ngControl?.`, so that one takes @optional() and stays nullable. ngforms 0 -> 253 passing, ngrouter 0 -> 85, ngtest 0 -> 48; all three suites build for the first time on this branch.
6281ad5 replaced `isNonNullableByDefault` in both places that decide whether the compiler emits null-safe syntax: //final isNullSafe = library.element.isNonNullableByDefault; final isNullSafe = !library.element.metadata.hasJS; `isNonNullableByDefault` was removed from the analyzer because every library is null safe under Dart 3, so it is always true. `metadata.hasJS` is an unrelated property -- whether the library carries a `@JS()` annotation -- and the two have nothing to do with one another. The effect is that any `@JS()` library gets pre-null-safety codegen, in which the emitter writes nullability as a comment rather than as syntax: static import3.ComponentStyles /*?*/ _componentStyles; static String /*?*/ get _debugComponentUrl { ... return null; } `ComponentStyles /*?*/` is simply `ComponentStyles`, so the generated file assigns null to non-nullable fields and returns null from non-nullable getters, and does not compile. `@GenerateInjector` output degrades the same way, emitting `_field0 ??= ...` against a non-nullable field and an `orElse` parameter that no longer matches the `Injector.injectFromSelfOptional` it overrides. Both sites become `true`, and the outliner's language-version prefix goes with them: it only ever inserted padding for the non-null-safe case. This is not confined to tests -- it degrades generated code for every `@JS()` library in any application. In this repo it blocked _tests/test/bootstrap/run_app_test.dart, the only test declaring an `@JS()` library.
6281ad5 narrowed `List<Object?> exports` back to `List<Object>`, undoing a widening 08baf6b had applied deliberately. `exports:` is never evaluated. `_extractExports` reads the AST of the list literal and recovers the identifier each element names, matching on `TypeLiteral` / `Identifier` / `PrefixedIdentifier` to reach its `staticElement`; what an element would evaluate to is irrelevant to the compiler. But the list is still ordinary Dart source that has to type check, so an exported constant whose value is null -- or whose static type is merely nullable -- cannot appear in a `List<Object>`. `Object?` is the widest type that lets any identifier be named. The narrowing forced `//lib.nullString,` to be commented out of the exports in additional_expression_test, which orphaned the `{{lib.nullString?.toUpperCase()}}` binding still present in that component's template. With nothing exported under that name the compiler resolved `lib` as a member of the component and reported "The getter 'lib' isn't defined for the type 'TestNullAwareFunctions'".
Twenty of the eighty-eight entrypoints in the browser preset did not compile, and because one bad entrypoint fails the whole build, none of the package's browser tests could run at all. Four distinct causes. Relative `lib/` imports (9 files). `import '../../../lib/matchers.dart'` crosses the lib/<->test/ boundary, which the VM resolves and DDC does not: they are separate modules, and build_runner reported it as "Error when reading 'lib/matchers.dart': File not found" even though the file was present. Switch these to `package:_tests/...` and drop the now-pointless `avoid_relative_lib_imports` suppressions. This alone fixed nine files. package:web and JS interop drift (7 files). Tear-offs of extension type interop members are disallowed, so `final select = element.querySelector` becomes a closure; `children.item(0)` and `querySelector` return nullable; `innerHTML` is `JSAny` in both directions, so writes take `.toJS` and reads come back through `(… as JSString?)?.toDart`; `Testability.whenStable` takes a `JSFunction`; `callMethod` with a list of arguments is now `callMethodVarArgs` with each argument converted. run_app_test additionally carried its own `@JS() abstract class JsTestability`, which the new static interop rules reject -- ngdart already exports an equivalent extension type, so use that. Deliberate nulls (1 file). mock_like_directive_test returns null from non-nullable members on purpose, to assert how a null `@Output` is handled on a mock-like versus a non-mock-like directive. Preserved as `null as dynamic` and `dynamic noSuchMethod(...)` so the test still asserts what it was written to. Framework defects (2 files), fixed in the two preceding commits: nullable `Component.exports`, and null-safe codegen for `@JS()` libraries. Also corrects an inverted loop bound in ng_class_test's `allCssClasses` helper. `for (var i = 0; i > c.length; i++)` is never entered, so the extension returned an empty list and every assertion built on it was vacuous. _tests browser preset: 68 of 88 entrypoints compiling and 475 tests passing -> 88 of 88 and 648 passing. The 141 remaining failures are behavioural and untouched here; most were simply unreachable before.
`_tests` declares `resolution: workspace`, so it has no independent dependency
resolution: with its entry commented out of the root `workspace:` list, `dart
pub get` inside it failed outright ("found no workspace root including it in
parent directories"), no package_config.json was generated, and nothing in the
package could be analysed, built or run. It was not skipped -- it was inert.
6281ad5 commented it out to get the Dart 3.12 refactor resolving without first
repairing ~40 files that no longer compiled. The cost was that the framework's
behavioural coverage -- core, di, linker, directives, templates, security --
stopped running entirely, which is why regressions in view teardown and in style
encapsulation reached this branch undetected: both are caught by tests that live
here.
The preceding commits make all 88 browser entrypoints compile, so restore it.
Note that CI has continued to generate six `_tests` jobs from
_tests/mono_pkg.yaml throughout; each began with `dart pub upgrade` in that
directory and so failed on the resolution error above. They will now run.
Browser preset currently: 648 passing, 11 skipped, 141 failing.
|
There are some fixes to this branch here: https://github.com/adamlofts/angular/tree/refactor/dart3.12 |
`_elementText` tested `n == Node`, comparing the value against the `Type` object
rather than asking whether it is one. That is never true for a DOM node, so
every node fell through to `return '$n'` and `hasTextContent` matched against
the element's `toString()`:
Expected: 1;2;3;4;5;
Actual: HtmlElement:<ngfor-hashcode-test>
The original read `n is Node`, which cannot be ported literally: the DOM types
in `package:web` are extension types over `JSObject`, so `is` erases to the
representation type and cannot tell them apart. Runtime discrimination has to go
through `isA`, as the same function already did one line below for `Element`.
Two further behaviours were lost in the same migration and are restored here:
`if (n is Comment) return '';` was dropped. Angular anchors `*ngIf` and `*ngFor`
with comment nodes, so they are on the child list of nearly every structural
directive, and their text would otherwise be concatenated into the result.
`childNodes` is a `NodeList`, which unlike `dart:html`'s `nodes` is not
`Iterable`. The recursive descent therefore never matched the `Iterable` branch
and stringified the list instead of walking it. Walk it by index.
Seven test files share this matcher. for_test goes 8/31 -> 31/31, and
ng_content_ref_test, projection_test, ng_template_outlet_test and
slice_pipe_test clear along with it.
Every assertion in the suite matched `Element.classList` against a
`List<String>`:
Expected: ['1']
Actual: DomTokenList:<1>
Which: is not Iterable
Under `dart:html` `classList` was a `CssClassSet`, which implemented `Iterable`,
so `equals(['foo'])` worked directly. Under `package:web` it is a
`DOMTokenList`, which does not. Add a `cssClasses` extension that reads the
tokens out by index and point the assertions at it.
This is on top of the inverted loop bound repaired in 96ed18c: the sibling
`allCssClasses` helper never entered its loop, so the handful of assertions
built on it were comparing against a list that was always empty.
35/35 passing.
All sixteen compiler_integration suites failed to load, so none of their tests
ran. Two helpers were responsible.
`test/resolve_util.dart` had two null-safety/API errors. An index expression is
not promoted by a null check, so `Platform.environment[...]` stayed `String?` at
the use site; read it into a local instead. And `loadPackageConfigUri` takes a
non-null `Uri` plus optional named parameters, so it can no longer be torn off
as the `Function(Uri?)` that `Isolate.packageConfig.then` wants; wrap it.
`lib/compiler.dart` was left half-migrated onto the current build_test API:
- It built `TemplateCompiler(BuilderOptions({}), null, null, null)`, but that
class is a compiler component, not a `Builder`. The builders are the
`templateCompiler` / `stylesheetCompiler` factories from
`package:ngdart/src/build.dart` that `build.yaml` itself registers. They
used to be composed with a `MultiplexingBuilder`, which no longer exists in
`build` 4 -- `testBuilders` accepts the list directly.
- It staged the user's sources into one `TestReaderWriter` and then handed
`testBuilder` a different, empty one, so the builder saw no input at all.
- The framework sources have to be readable, or no `@Component` annotation
resolves and the tests observe "Could not resolve ..." instead of the
diagnostic they assert on. They must equally not be *built*. `testBuilders`
derives the packages it builds from `sourceAssets` and reads everything else
from `readerWriter`, so `package:ngdart` belongs in the latter; passing it
as a source instead makes the builders compile the whole framework.
_tests VM preset: 175 passing with 21 files failing to load -> 195 passing with
5. The five remaining are unrelated files with their own unmigrated null-safety
and analyzer-13 errors.
This makes the suites run; it does not make them pass. 47 assertions still fail,
all the same way: the expected diagnostic is emitted, but each error is recorded
twice -- once as the diagnostic and once more carrying a stack trace, from
`_TestCompileContext.reportAndRecover` throwing rather than recovering -- so an
expectation written against a single record now sees two. Left failing.
`_setProperty` called `_ngElement?.setProperty(...)`, which resolves to the
extension from `dart:js_interop_unsafe` -- it defines a JavaScript field of that
name on the DOM object. `[ngStyle]="{'max-width': '40px'}"` therefore set a
property literally called `max-width` on the element and changed nothing about
how it rendered. NgStyle did not work at all.
The line it replaced, left commented out directly above it, was
`_ngElement.style.setProperty(...)`, which sets a CSS property on the inline
style declaration. It could not be ported as written: `style` is declared on
`HTMLElement` and `SVGElement` rather than on `Element`, and the injected host
element is typed `Element?`, so the property is not reachable from the static
type. Resolve it through `isA` instead.
A removed key reaches `_setProperty` with a null value, since the differ nulls
`currentValue` when it moves a record to the removals list. Pass the empty
string in that case, which clears the declaration -- the behaviour `dart:html`
gave for a null value.
_tests/test/common/directives/ng_style_test.dart: 0/4 -> 4/4.
The guard read `assert(child == Element, ...)`, comparing the argument against the `Element` *type object* rather than asking whether it is one. That is never true for any real argument, so the assertion failed on every call and `markChildForCheck` threw for every `@ContentChild`/`@ViewChild` query that tried to mark its child for check. Release builds strip asserts, so this only bit in development -- which is where the affected code paths are exercised. The intent is preserved in the line commented out above it, `assert(child is! Element, ...)`. That form cannot be restored verbatim either: `Element` is a JS interop extension type, so `is` erases to its representation type and the analyzer flags the check as not platform-consistent. Narrow to `JSObject` and use `isA` for the actual instanceof test. This is the same rewrite that broke `_elementText` in _tests/lib/matchers.dart, repaired in f5341b6: an `is` check turned into `==` against a type literal. _tests/test/core/change_detection/mark_child_for_check_test.dart: 0/8 -> 8/8.
…rtion Style encapsulation itself was working -- the rendered markup in the failure output carried both the class and its `_ngcontent-` shim attribute. Two defects in the test accounted for all three failures. `tearDown` walked the style elements as `for (var i = el.length; i > 0; i--) el.item(i)`, reading one past the end on the first iteration. `item` returns null there, the cast threw, and the throw happened before `disposeAnyRunningTest()` ran -- so the fixture stayed live and the following two tests failed with "Another instance of an `NgTestFixture` is still executing!" rather than on their own merits. One off-by-one, three failures. The `[attr.class]` test had its `getComputedStyle().position` assertion commented out and replaced with `element.style.backgroundPosition`: a different property, and an inline style rather than the stylesheet rule the test exists to check. Restored to match its `[class]` sibling. 3/3 passing.
Both suites interrogated DOM collections with `contains` and `containsPair`:
expect(hostElement.attributes, containsPair('title', 'inherited'));
expect(element.classList, contains('fancy'));
Under `dart:html` those were a `Map<String, String>` and an `Iterable<String>`.
Under `package:web` they are `NamedNodeMap` and `DOMTokenList`, which are
neither, so the matcher failed on the collection rather than on its contents --
`NoSuchMethodError: 'containsKey'` on `_NamedNodeMap`, and "is not a string, map
or iterable" for `DOMTokenList`.
Nothing was wrong with what the framework rendered, and the failure output said
so: the conditional-attribute case reported `Actual: [_Attr:disabled, ...]` and
the conditional-class case `Actual: DomTokenList:<fancy>`. The values were
right; the collections just could not be asked. `@HostBinding` and metadata
inheritance behave correctly for application code.
Add `attributeMap` and `cssClasses` to the shared matchers library, alongside
the equivalent already inlined in ng_class_test, and point the assertions at
them so they keep their original shape.
directive_inheritance_test 17/22 -> 22/22, host_annotation_test 12/15 -> 15/15.
Same drift as d86cd4d, in the remaining places that reached for a DOM collection as though it were a Dart one. `NamedNodeMap` has no `containsKey` and `DOMTokenList` is not `Iterable`, so `containsPair` and `contains` failed against the collection rather than against its contents. Again nothing was wrong with what was rendered -- the class-alias case reported `Actual: DomTokenList:<foo bar>` while asserting it contained 'foo'. Attribute binding, attribute removal on a null expression, and the `class` alias all behave correctly for application code. Swept the rest of the suite for the same pattern; these were the last of it. binding_integration_test 7/10 -> 10/10, plus one assertion each in misc_test and view_creation_test.
None of these were framework defects; each was a test that stopped asking the
question it was written to ask.
template_test `childNodes.item(0)` is the `<template>` anchor. The
assertion for 'hello' had been moved from index 1 to 0,
so it read the anchor's empty text.
outputs_test `MouseEventInit(cancelable: true)` was dropped from the
event constructor. `preventDefault()` does nothing on a
non-cancelable event, so `defaultPrevented` stayed false.
The removed comment noted `dart:html` defaulted it true.
directive_integration_test
`DuplicateDir(HtmlElement)` no longer names a DI token
the compiler can resolve -- it erases to `JSObject` and
the lookup failed. Restore `HTMLElement` and
`textContent`.
injector_test `CaptureInjectInjector.injectFromSelfOptional` was
changed to `return orElse`, which means "not found", so
`get` threw before the assertions ran. It must return
`null`: "found, and the value is null".
key_events_test The guard injecting the keyboard-event helper script was
inverted -- it added the script only when the function it
defines already existed, so the first call always found
it undefined.
run_app_test `h1.style.height` reads inline styles; the height comes
from the component's `styles:` block and is only visible
on the computed style. The commented-out original used
`getComputedStyle`.
security_integration_test
`NgTestBed<T>` lost its type argument, so `update`'s
callback parameter was untyped and each test cast the
*component instance* to a DOM element and assigned to the
element instead of the component's field. Restored the
type arguments and the field assignments.
Browser preset: 746 passing / 43 failing -> 761 / 28.
The 28 that remain are not test defects and are left alone: 21 from the HTML
sanitizer returning its input unchanged, 5 from `createTrustedHtml` appending
markup as a text node rather than parsing it, and 2 in mock_like_directive_test
whose premise -- a null value behind a non-nullable `@Output` -- is no longer
expressible under sound null safety.
`createTrustedHtml` built an empty fragment and then did
doc.append(sanitizeHtml(trustedHtml).toJS);
`ParentNode.append` given a string inserts it as a *text node*. The markup was
therefore never parsed, and its tags rendered as visible characters: an i18n
message declared as "A message<br> with <strong>emphasis!</strong>" reached the
page with the tags shown literally rather than as a line break and bold text.
The same path backs `[safeInnerHtml]`, so trusted values there were not parsed
either.
It also ran `sanitizeHtml` over content the caller had already established as
trusted, which is the opposite of this function's contract -- the
`NodeTreeSanitizer.trusted` argument in the `dart:html` original said exactly
that.
Parse through a `<template>`: its `content` is already a `DocumentFragment`, and
the fragment parsing algorithm does not impose the usual element-nesting
restrictions inside one, so markup like a bare `<tr>` survives where parsing via
a `<div>` would discard it. `setHTMLUnsafe` is the parse-without-sanitizing
primitive, which is what is wanted here; it needs no new dependency, and the
`sanitize_html` import this file no longer uses goes with it.
This does not address the separate defect in `sanitizeHtmlInternal`, which
returns its input unchanged and leaves `[innerHtml]` unsanitized.
_tests/test/core/i18n_test.dart: 8/12 -> 12/12. Browser preset 761 -> 765.
… text
The directive assigned the trusted markup to `textContent`, so it was inserted
as a text node and its tags rendered as visible characters. The call it replaced
is commented out directly above it:
//_element.setInnerHtml(
// safeInnerHtml.changingThisWillBypassSecurityTrust,
// treeSanitizer: NodeTreeSanitizer.trusted,
//);
`setHTMLUnsafe` is the parse-without-sanitizing primitive, which is exactly what
`NodeTreeSanitizer.trusted` asked for, and what this directive exists to do --
its whole contract is that the caller has already vouched for the markup.
`package:web` binds it on `Element`, so no interop is needed.
Third instance of this defect after `removeNodes` (d1140a5) and
`createTrustedHtml` (1f306dc): a DOM call replaced during the `package:web`
migration by one that compiles, reads plausibly, and does something else.
test/security/safe_inner_html_test.dart still has one failure after this, from
the separate defect in `sanitizeHtmlInternal`, fixed next.
`sanitizeHtmlInternal` assigned its input to `textContent` and read the same
property straight back, which is an identity function. It returned untrusted
markup verbatim, so `[innerHtml]` -- and any caller of the public
`DomSanitizationService.sanitizeHtml` -- performed no sanitization at all:
should escape unsafe HTML
Expected: 'ha '
Actual: 'ha <script>evil()</script>'
Delegate to `Element.setHTML` on a detached element and read `innerHTML` back
off it. The allow-list is then the browser's own and tracks its security
updates, rather than being pinned to a package version. This drops the
`sanitize_html` dependency, whose last use went with 1f306dc.
The configuration is deliberately left empty. Passing one does not extend the
default -- it *replaces* it, and a config that names no elements permits
everything: with `{sanitizer: {comments: true}}` the suite reported `alt`,
`xlink:href`, `<my:hr>` and `<param>` all surviving. Every relaxation
(`allowElement`, `allowAttribute`, `setComments`) is a security decision, and
the unconfigured default is the one browser vendors maintain.
`Element.setHTML` is not bound by `package:web` yet, hence `callMethodVarArgs`.
It requires Chrome 140, Firefox 145 or Safari 26.2; on anything older this
throws rather than silently returning unsanitized markup.
Three expectations move to record the browser's allow-list, which is stricter
than `NodeTreeSanitizer`'s was:
- comments are stripped rather than preserved;
- `<img>` is dropped outright, attributes and all -- it is the classic
`onerror` vector and is absent from the default allow-list;
- `<iframe>` likewise, taking its content with it.
Markup needing images or frames is trusted markup, and belongs in
`[safeInnerHtml]`, which does not sanitize.
Browser preset: 765 passing / 24 failing -> 787 / 2.
`resolveToBound` moved from `DartType` to `TypeSystem` during the analyzer migration, and the two take opposite arguments: the old one took the bound to fall back to, the new one takes the type to resolve. 6f78e29 kept passing the fallback, so `typeSystem.resolveToBound(dynamicType)` resolved `dynamic` to itself and every type parameter collapsed to `dynamic` -- the next line's `is DynamicType` check then returned `o.dynamicType` unconditionally. This is the same misuse fixed in `find_components.dart` by 460d376, and takes the same shape here: resolve `dartType` itself, and keep the explicit `dynamic` fallback for an unbounded parameter, which `resolveToBound` would otherwise resolve to `Object?`. Making the branch yield a non-nullable type restores promotion for the rest of the function, so the three `?.` it had to carry become redundant. Note this branch is only reachable for a *bare* top-level type parameter: the type-argument loop below short-circuits type parameters to `dynamic` before they reach it, to avoid a stack overflow on `<T extends List<T>>`. Regenerating the whole goldens corpus before and after produces a byte-identical diff, and an instrumented build never enters the branch. Covered by a unit test instead, which fails on the old code with `Actual: BuiltinType` where `num` is expected.
The sibling half of 5a8abbe. `provideTokenOptional` is documented as returning null when no provider is found, but its body is identical to `provideToken` -- `get(token)` with no not-found value, so it throws. Same fix: return `T?`, and pass `null` as the not-found value. `provideUntyped` compares the self lookup against `orElse` rather than against the `throwIfNotFound` sentinel, so a null not-found value still falls through to the ancestry walk. No callers today, in this repo or in ngcomponents, so the signature change breaks nothing. Verified on the VM: null for a missing token, the value for a provided one, and a parent injector's value from a child that does not provide it.
The symmetric half of 29c3453, which fixed the `read:` token but left the query *selector* -- the `Type` in `@ViewChild(Foo)` -- on a raw `toTypeValue()`. Both are matched by identity against what the element publishes, so both need the un-erased type. For a `package:web` type the erased value is `dart:_interceptors`' `JSObject`, which matches nothing, and which puts a private SDK import into the generated `.template.dart` -- the failure that makes `build_web_compilers` skip every entrypoint reaching it while reporting an empty list of offending libraries. See `unerasedTypeValueOf`. Only extension types are affected; a directive selector is a plain class and resolves exactly as before. ngcomponents queries by `read:` rather than by a `package:web` selector type, so nothing downstream changes today -- this is to stop the two paths drifting.
Refactor/dart3.12
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
3 participants
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.
Majorly refactored with breaking changes that involves the following:
dart:htmlanddart:jswithpackage:webanddart:js_interopanalyzerto 13.0.0All test cases passed but more testings are needed. An upgraded version of
ngcomponentshas successfully passed compilation using this build.