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

I18n extractor avoid standalone filesystem api #39006

Closed
wants to merge 3 commits into from
Closed

I18n extractor avoid standalone filesystem api #39006

wants to merge 3 commits into from

Conversation

petebacondarwin
Copy link
Member

This should fix Windows source-mapping in the i18n extraction integration with CLI.

I have tried to ensure that the public API items are backward compatible with what the CLI currently uses.
Ideally the CLI will switch over to providing the file-system object where needed but I am not sure if this is entirely necessary to fix the reported problem.

FIxes #38711

@petebacondarwin petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer area: compiler Issues related to `ngc`, Angular's template compiler area: i18n target: patch This PR is targeted for the next patch release type: bug/fix refactoring Issue that involves refactoring or code-cleanup labels Sep 26, 2020
@ngbot ngbot bot modified the milestone: needsTriage Sep 26, 2020
@petebacondarwin
docs(localize): add public api markers for CLI integration
3426f58 
This commit marks the functions and classes that are
used by the CLI.
@petebacondarwin
refactor(compiler-cli): avoid free-standing FileSystem functions
45de7a8 
These free standing functions rely upon the "current" `FileSystem`,
but it is safer to explicitly pass the `FileSystem` into functions or
classes that need it.
@petebacondarwin petebacondarwin force-pushed the i18n-extractor-avoid-standalone-filesystem-api branch from 740b85e to a9e0105 Compare September 26, 2020 13:42
@petebacondarwin
refactor(localize): avoid free-standing FileSystem functions
ca89d35 
These free standing functions rely upon the "current" `FileSystem`,
but it is safer to explicitly pass the `FileSystem` into functions or
classes that need it.

Fixes angular#38711
@petebacondarwin petebacondarwin force-pushed the i18n-extractor-avoid-standalone-filesystem-api branch from a9e0105 to ca89d35 Compare September 26, 2020 14:00
@alan-agius4
Copy link
Contributor

I testing the fix and this does indeed address the sourcemap issue on Windows.

alan-agius4
alan-agius4 approved these changes Sep 28, 2020
View reviewed changes
Copy link
Contributor

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

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

LGTM

@alan-agius4 alan-agius4 mentioned this pull request Sep 28, 2020
Merged
alan-agius4 added a commit to angular/angular-cli that referenced this pull request Sep 28, 2020
@alan-agius4
fix(@angular-devkit/build-angular): implement relative method in ex…
1c4d358 
…tractor FS

This will be needed when the changes in angular/angular#39006 is merged.
clydin
clydin reviewed Sep 28, 2020
View reviewed changes
packages/compiler-cli/src/ngtsc/sourcemaps/test/source_file_spec.ts
@@ -7,17 +7,19 @@
*/
import {encode} from 'sourcemap-codec';

import {absoluteFrom} from '../../file_system';
import {absoluteFrom, FileSystem, getFileSystem} from '../../file_system';
import {runInEachFileSystem} from '../../file_system/testing';
import {RawSourceMap} from '../src/raw_source_map';
import {SegmentMarker} from '../src/segment_marker';
import {computeStartOfLinePositions, ensureOriginalSegmentLinks, extractOriginalSegments, findLastMappingIndexBefore, Mapping, parseMappings, SourceFile} from '../src/source_file';

runInEachFileSystem(() => {
Copy link
Member

@clydin clydin Sep 28, 2020 •
edited

Choose a reason for hiding this comment

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

I think we should consider having runInEachFileSystem pass the file system instance currently under test in the callback. From a quick search, this would remove the need for a large amount of getFileSystem calls.
packages/compiler-cli/ngcc/test/dependencies/module_resolver_spec.ts alone has almost 20 calls to getFileSystem.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I was thinking exactly that while working on this. But I felt it was too large a change for this PR. Let's do it in a follow up PR.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed.

@AndrewKushnir AndrewKushnir removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Sep 29, 2020
@AndrewKushnir
Copy link
Contributor

FYI, this change does not require g3 presubmit since we do not sync the packages/compiler-cli/src/ngtsc/sourcemaps and packages/localize folders to Google's codebase. I've created a tiny PR to update NgBot config to reflect that (corresponding change on g3 side is already in place).

@petebacondarwin petebacondarwin added the action: merge The PR is ready for merge by the caretaker label Sep 30, 2020
josephperrott pushed a commit that referenced this pull request Sep 30, 2020
@petebacondarwin @josephperrott
refactor(compiler-cli): avoid free-standing FileSystem functions (#…
ac3aa04 
…39006)

These free standing functions rely upon the "current" `FileSystem`,
but it is safer to explicitly pass the `FileSystem` into functions or
classes that need it.

PR Close #39006
josephperrott pushed a commit that referenced this pull request Sep 30, 2020
@petebacondarwin @josephperrott
refactor(localize): avoid free-standing FileSystem functions (#39006)
79620f5 
These free standing functions rely upon the "current" `FileSystem`,
but it is safer to explicitly pass the `FileSystem` into functions or
classes that need it.

Fixes #38711

PR Close #39006
josephperrott pushed a commit that referenced this pull request Sep 30, 2020
@petebacondarwin @josephperrott
docs(localize): add public api markers for CLI integration (#39006)
965249d 
This commit marks the functions and classes that are
used by the CLI.

PR Close #39006
josephperrott pushed a commit that referenced this pull request Sep 30, 2020
@petebacondarwin @josephperrott
refactor(compiler-cli): avoid free-standing FileSystem functions (#…
669e075 
…39006)

These free standing functions rely upon the "current" `FileSystem`,
but it is safer to explicitly pass the `FileSystem` into functions or
classes that need it.

PR Close #39006
josephperrott pushed a commit that referenced this pull request Sep 30, 2020
@petebacondarwin @josephperrott
refactor(localize): avoid free-standing FileSystem functions (#39006)
1b5f6ee 
These free standing functions rely upon the "current" `FileSystem`,
but it is safer to explicitly pass the `FileSystem` into functions or
classes that need it.

Fixes #38711

PR Close #39006
@petebacondarwin petebacondarwin deleted the i18n-extractor-avoid-standalone-filesystem-api branch October 1, 2020 12:49
@petebacondarwin petebacondarwin mentioned this pull request Oct 7, 2020
Closed
15 tasks
@janousek janousek mentioned this pull request Oct 8, 2020
Closed
mgechev pushed a commit to angular/angular-cli that referenced this pull request Oct 8, 2020
@alan-agius4 @mgechev
fix(@angular-devkit/build-angular): implement relative method in ex…
a4b6eca 
…tractor FS

This will be needed when the changes in angular/angular#39006 is merged.

(cherry picked from commit 1c4d358)

Closes: #19005
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alan-agius4 alan-agius4 alan-agius4 approved these changes

@clydin clydin clydin approved these changes

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler area: i18n cla: yes refactoring Issue that involves refactoring or code-cleanup target: patch This PR is targeted for the next patch release type: bug/fix
Projects
None yet
Milestone
needsTriage
Development

Successfully merging this pull request may close these issues.

Extractor no longer extracts the parameter/expression of the interpolated string into disp attribute
5 participants
@petebacondarwin @alan-agius4 @AndrewKushnir @clydin @googlebot