Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(@schematics/angular): use isolatedModules in generated project #27988

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(@schematics/angular): use isolatedModules in generated project #27988

wants to merge 1 commit into from

Conversation

cexbrayat
Copy link
Member

PR Checklist

Please check to confirm your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

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

What is the new behavior?

As 3acb776 introduced an optimization for projects that use isolatedModules, this commit makes projects generated with ng new use this option by default.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@angular-robot angular-robot bot added the detected: feature PR contains a feature commit label Jul 3, 2024
@alan-agius4 alan-agius4 requested a review from clydin July 3, 2024 14:44
@cexbrayat
Copy link
Member Author

It looks like only one i18n test is failing (extract-ivy-library) but only when running with esbuild enabled:

It seems that in that case the messages from app.component.html are referenced as src/app/app.component.ts

Test Process error Error: File "messages.xlf" did not contain "src/app/app.component.html"...
Content:
<?xml version="1.0" encoding="UTF-8" ?>
<xliff version="1.2" xmlns="urn:oasis:names:tc:xliff:document:1.2">
  <file source-language="en-US" datatype="plaintext" original="ng2.template">
    <body>
      <trans-unit id="5640424602650517303" datatype="html">
        <source> i18n-lib-test works! </source>
        <context-group purpose="location">
          <context context-type="sourcefile">projects/i18n-lib-test/src/lib/i18n-lib-test.component.ts</context>
          <context context-type="linenumber">9,10</context>
        </context-group>
      </trans-unit>
      <trans-unit id="2023484548631819319" datatype="html">
        <source>Hello world</source>
        <context-group purpose="location">
          <context context-type="sourcefile">src/app/app.component.ts</context>
          <context context-type="linenumber">16</context>
        </context-group>
      </trans-unit>
    </body>
  </file>
</xliff>

@clydin
Copy link
Member

clydin commented Jul 3, 2024

Yes. I re-ran it so it's not a flake.
It appears this will need further investigation.

@cexbrayat
Copy link
Member Author

Yeah, I confirm that this is an issue: I just ran ng extract-i18n on a project of mine with isolatedModules: true, and the extracted messages only reference the TS files instead of the HTML files, line numbers are off, and equiv-text is broken.

@clydin
Copy link
Member

clydin commented Jul 3, 2024 •
edited
Loading

#27992 should fix the underlying issue.

@cexbrayat
feat(@schematics/angular): use isolatedModules in generated project
5cb736d 
As 3acb776 introduced an optimization for projects that use `isolatedModules`,
this commit makes projects generated with `ng new` use this option by default.
@cexbrayat
Copy link
Member Author

@clydin I rebased on your fix, and the PR is now green 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clydin clydin Awaiting requested review from clydin

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
detected: feature PR contains a feature commit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@cexbrayat @clydin