feat: 🎸 Add support for self closing tag and control flow

[kekel87]

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/ngneat/transloco/blob/master/CONTRIBUTING.md#commit
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[x] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

The library does not work with templates containing self-closing (Angular 15) components and control flows (Angular 17).

Issue Number: #171, #155

What is the new behavior?

Self-closing components work without changing anything other than updating the compiler.
I added processing to pass through Control Flows (BlockNode).

Does this PR introduce a breaking change?

[ ] Yes
[x] No

🤷 Perhaps old versions of Angular that are no longer supported via the latest compiler.

Other information

• implement function to detect BlockNode and get children of BlockNode
• add unit test to self-closing tag and control flow
• since you haven't yet merged the infra/migrate-to-esm branch, you only had to look at the last commit

[shaharkazaz]

I'm currently on vacation, when I get back I'll review 🤗

[jabiinfante]

Hi!

Thanks for your work upgrading the library to support new angular features.
As far as i have try, it works like a charm.

I have found a little issue using marker:

import { marker } from '@ngneat/transloco-keys-manager/marker';

It seems the entrypoint is not found in the new package.json esm version:

Error: Module not found: Error: Package path ./marker is not exported from package

Changing the exports key in the package.json like theis, seems to work as expected:

  "exports": {
    ".": {
      "import": "./public-api.js",
      "types": "./public-api.d.ts"
    },
    "./marker": {
      "import": "./marker.js",
      "types": "./marker.d.ts"
    }
  },

Hope it helps.

[shaharkazaz]

@jabiinfante Thanks!
@kekel87 Can you please add this to the package.json?

[shaharkazaz]

Please separate this into isBlockNode and resolveBlockChildren
Once separated we can make node.children the default return value of resolveBlockChildren and have something like this:

    if (isBlockNode(node)) {
      traverse(resolveBlockChildNodes(node), containers, config);
      continue;
    }

[kekel87]

This means we'll have to run the if tests twice. Not very optimizing, is it?

What's more, if I return node.children without doing an if, I get a typing error.

export function isBlockNode(node: TmplAstNode): boolean {
  // test here for the first time
  return isTmplAstIfBlock(node) ||
    isTmplAstForLoopBlock(node) ||
    isTmplAstDeferredBlock(node) ||
    isTmplAstSwitchBlock(node) ||
    isBlockWithChildren(node);
}

export function resolveBlockChildNodes(node: TmplAstNode): TmplAstNode[] {
  // test again
  if (isTmplAstIfBlock(node)) {
    return node.branches;
  }

  if (isTmplAstForLoopBlock(node)) {
    return node.empty ? [...node.children, node.empty] : node.children;
  }

  if (isTmplAstDeferredBlock(node)) {
    // Typing errors here
    return [
      ...node.children,
      ...[node.loading, node.error, node.placeholder].filter(Boolean),
    ];
  }

  if (isTmplAstSwitchBlock(node)) {
    return node.cases;
  }

   // Typing errors here
  return node.children;
}

[shaharkazaz]

1. We don't have any performance considerations here and this isn't some very heavy calculation that I would agree we need to optimize, I'd take readability.
2. What's the type error?

[kekel87]

Done

[shaharkazaz]

This can be removed, see the next comments

[kekel87]

It's a great tool for solving typing problems in strict mode. See #180 (comment) and #180 (comment)

[shaharkazaz]

K I see.
Let's a positive form of this like isTmplAstNode instead of isNotNull

[kekel87]

Done

[kekel87]

@jabiinfante Thanks! @kekel87 Can you please add this to the package.json?

Done, I have also include this fix: #132

[shaharkazaz]

@kekel87 Just making sure you saw my last comments :)

[shaharkazaz]

Why did you update the signature? do we support resolving params like lang?

[kekel87]

I don't know what to say. You saw my comment 😅

This is a way of specifying the scope.
I don't think this is a definitive solution.
But it does get people unstuck.

It comes from resolving the marker function in the same way as the Transloco service and pipe.

Code

  if (content.includes('@ngneat/transloco')) {
    extractors.push(serviceExtractor, pureFunctionExtractor);
  }

  if (content.includes('@ngneat/transloco-keys-manager')) {
    extractors.push(markerExtractor);
  }

  ...

  extractors
    .map((ex) => ex(ast))
    .flat()
    .forEach(({ key, lang }) => {
      // Here, we solve three functions in the same way.
      const [keyWithoutScope, scopeAlias] = resolveAliasAndKeyFromService(
        key,
        lang,
        scopes
      );
      addKey({
        scopeAlias,
        keyWithoutScope,
        ...baseParams,
      });
    });

Maybe there's a way to improve this. You need to support the scope in marker in any case. #178 #132 #179
But I grant you it has nothing to do with this PR. I can remove it if you like.

I'd like to finish the Angular subject quickly. 😅

[kekel87]

I've just reverted to this change.
I propose to move forward and I was preparing another PR for that.
But after the release of a version.

[shaharkazaz]

@kekel87 Missed the ref to the other issue.
I think it should be in another PR as you intended 👍

[kekel87]

@shaharkazaz Hi, any plan for a new release ?

[shaharkazaz]

@kekel87 yes, I need to finish something in the Transloco repo

[shaharkazaz]

@kekel87 v4 is released. Thank you for all your contribution 👏