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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3426f58
docs(localize): add public api markers for CLI integration
petebacondarwin Sep 26, 2020
45de7a8
refactor(compiler-cli): avoid free-standing `FileSystem` functions
petebacondarwin Sep 26, 2020
ca89d35
refactor(localize): avoid free-standing `FileSystem` functions
petebacondarwin Sep 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 7 additions & 5 deletions packages/compiler-cli/src/ngtsc/sourcemaps/src/source_file.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import {removeComments, removeMapFileComments} from 'convert-source-map';
import {decode, encode, SourceMapMappings, SourceMapSegment} from 'sourcemap-codec';

import {AbsoluteFsPath, dirname, relative} from '../../file_system';
import {AbsoluteFsPath, FileSystem} from '../../file_system';

import {RawSourceMap} from './raw_source_map';
import {compareSegments, offsetSegment, SegmentMarker} from './segment_marker';
Expand Down Expand Up @@ -38,7 +38,9 @@ export class SourceFile {
/** Whether this source file's source map was inline or external. */
readonly inline: boolean,
/** Any source files referenced by the raw source map associated with this source file. */
readonly sources: (SourceFile|null)[]) {
readonly sources: (SourceFile|null)[],
private fs: FileSystem,
) {
this.contents = removeSourceMapComments(contents);
this.startOfLinePositions = computeStartOfLinePositions(this.contents);
this.flattenedMappings = this.flattenMappings();
Expand Down Expand Up @@ -75,11 +77,11 @@ export class SourceFile {
mappings[line].push(mappingArray);
}

const sourcePathDir = dirname(this.sourcePath);
const sourcePathDir = this.fs.dirname(this.sourcePath);
const sourceMap: RawSourceMap = {
version: 3,
file: relative(sourcePathDir, this.sourcePath),
sources: sources.map(sf => relative(sourcePathDir, sf.sourcePath)),
file: this.fs.relative(sourcePathDir, this.sourcePath),
sources: sources.map(sf => this.fs.relative(sourcePathDir, sf.sourcePath)),
names,
mappings: encode(mappings),
sourcesContent: sources.map(sf => sf.contents),
Expand Down
6 changes: 3 additions & 3 deletions packages/compiler-cli/src/ngtsc/sourcemaps/src/source_file_loader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
*/
import {commentRegex, fromComment, mapFileCommentRegex} from 'convert-source-map';

import {absoluteFrom, AbsoluteFsPath, FileSystem} from '../../file_system';
import {AbsoluteFsPath, FileSystem} from '../../file_system';
import {Logger} from '../../logging';

import {RawSourceMap} from './raw_source_map';
Expand Down Expand Up @@ -83,7 +83,7 @@ export class SourceFileLoader {
inline = mapAndPath.mapPath === null;
}

return new SourceFile(sourcePath, contents, map, inline, sources);
return new SourceFile(sourcePath, contents, map, inline, sources, this.fs);
} catch (e) {
this.logger.warn(
`Unable to fully load ${sourcePath} for source-map flattening: ${e.message}`);
Expand Down Expand Up @@ -124,7 +124,7 @@ export class SourceFileLoader {
}
}

const impliedMapPath = absoluteFrom(sourcePath + '.map');
const impliedMapPath = this.fs.resolve(sourcePath + '.map');
if (this.fs.exists(impliedMapPath)) {
return {map: this.readRawSourceMap(impliedMapPath), mapPath: impliedMapPath};
}
Expand Down
69 changes: 37 additions & 32 deletions packages/compiler-cli/src/ngtsc/sourcemaps/test/source_file_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

describe('SourceFile and utilities', () => {
let fs: FileSystem;
let _: typeof absoluteFrom;

beforeEach(() => {
fs = getFileSystem();
_ = absoluteFrom;
});

Expand All @@ -40,7 +42,7 @@ runInEachFileSystem(() => {
sources: ['a.js'],
version: 3
};
const originalSource = new SourceFile(_('/foo/src/a.js'), 'abcdefg', null, false, []);
const originalSource = new SourceFile(_('/foo/src/a.js'), 'abcdefg', null, false, [], fs);
const mappings = parseMappings(rawSourceMap, [originalSource], [0, 8]);
expect(mappings).toEqual([
{
Expand Down Expand Up @@ -71,7 +73,8 @@ runInEachFileSystem(() => {

it('should parse the segments in ascending order of original position from the raw source map',
() => {
const originalSource = new SourceFile(_('/foo/src/a.js'), 'abcdefg', null, false, []);
const originalSource =
new SourceFile(_('/foo/src/a.js'), 'abcdefg', null, false, [], fs);
const rawSourceMap: RawSourceMap = {
mappings: encode([[[0, 0, 0, 0], [2, 0, 0, 3], [4, 0, 0, 2]]]),
names: [],
Expand All @@ -88,8 +91,8 @@ runInEachFileSystem(() => {
});

it('should create separate arrays for each original source file', () => {
const sourceA = new SourceFile(_('/foo/src/a.js'), 'abcdefg', null, false, []);
const sourceB = new SourceFile(_('/foo/src/b.js'), '1234567', null, false, []);
const sourceA = new SourceFile(_('/foo/src/a.js'), 'abcdefg', null, false, [], fs);
const sourceB = new SourceFile(_('/foo/src/b.js'), '1234567', null, false, [], fs);
const rawSourceMap: RawSourceMap = {
mappings:
encode([[[0, 0, 0, 0], [2, 1, 0, 3], [4, 0, 0, 2], [5, 1, 0, 5], [6, 1, 0, 2]]]),
Expand Down Expand Up @@ -313,8 +316,8 @@ runInEachFileSystem(() => {
describe('ensureOriginalSegmentLinks', () => {
it('should add `next` properties to each segment that point to the next segment in the same source file',
() => {
const sourceA = new SourceFile(_('/foo/src/a.js'), 'abcdefg', null, false, []);
const sourceB = new SourceFile(_('/foo/src/b.js'), '1234567', null, false, []);
const sourceA = new SourceFile(_('/foo/src/a.js'), 'abcdefg', null, false, [], fs);
const sourceB = new SourceFile(_('/foo/src/b.js'), '1234567', null, false, [], fs);
const rawSourceMap: RawSourceMap = {
mappings:
encode([[[0, 0, 0, 0], [2, 1, 0, 3], [4, 0, 0, 2], [5, 1, 0, 5], [6, 1, 0, 2]]]),
Expand All @@ -336,14 +339,14 @@ runInEachFileSystem(() => {
describe('flattenedMappings', () => {
it('should be an empty array for source files with no source map', () => {
const sourceFile =
new SourceFile(_('/foo/src/index.js'), 'index contents', null, false, []);
new SourceFile(_('/foo/src/index.js'), 'index contents', null, false, [], fs);
expect(sourceFile.flattenedMappings).toEqual([]);
});

it('should be empty array for source files with no source map mappings', () => {
const rawSourceMap: RawSourceMap = {mappings: '', names: [], sources: [], version: 3};
const sourceFile =
new SourceFile(_('/foo/src/index.js'), 'index contents', rawSourceMap, false, []);
new SourceFile(_('/foo/src/index.js'), 'index contents', rawSourceMap, false, [], fs);
expect(sourceFile.flattenedMappings).toEqual([]);
});

Expand All @@ -355,25 +358,26 @@ runInEachFileSystem(() => {
sources: ['a.js'],
version: 3
};
const originalSource = new SourceFile(_('/foo/src/a.js'), 'abcdefg', null, false, []);
const originalSource =
new SourceFile(_('/foo/src/a.js'), 'abcdefg', null, false, [], fs);
const sourceFile = new SourceFile(
_('/foo/src/index.js'), 'abc123defg', rawSourceMap, false, [originalSource]);
_('/foo/src/index.js'), 'abc123defg', rawSourceMap, false, [originalSource], fs);
expect(removeOriginalSegmentLinks(sourceFile.flattenedMappings))
.toEqual(parseMappings(rawSourceMap, [originalSource], [0, 11]));
});

it('should merge mappings from flattened original source files', () => {
const cSource = new SourceFile(_('/foo/src/c.js'), 'bcd123', null, false, []);
const dSource = new SourceFile(_('/foo/src/d.js'), 'aef', null, false, []);
const cSource = new SourceFile(_('/foo/src/c.js'), 'bcd123', null, false, [], fs);
const dSource = new SourceFile(_('/foo/src/d.js'), 'aef', null, false, [], fs);

const bSourceMap: RawSourceMap = {
mappings: encode([[[0, 1, 0, 0], [1, 0, 0, 0], [4, 1, 0, 1]]]),
names: [],
sources: ['c.js', 'd.js'],
version: 3
};
const bSource =
new SourceFile(_('/foo/src/b.js'), 'abcdef', bSourceMap, false, [cSource, dSource]);
const bSource = new SourceFile(
_('/foo/src/b.js'), 'abcdef', bSourceMap, false, [cSource, dSource], fs);

const aSourceMap: RawSourceMap = {
mappings: encode([[[0, 0, 0, 0], [2, 0, 0, 3], [4, 0, 0, 2], [5, 0, 0, 5]]]),
Expand All @@ -382,7 +386,7 @@ runInEachFileSystem(() => {
version: 3
};
const aSource =
new SourceFile(_('/foo/src/a.js'), 'abdecf', aSourceMap, false, [bSource]);
new SourceFile(_('/foo/src/a.js'), 'abdecf', aSourceMap, false, [bSource], fs);

expect(removeOriginalSegmentLinks(aSource.flattenedMappings)).toEqual([
{
Expand Down Expand Up @@ -431,15 +435,16 @@ runInEachFileSystem(() => {
sources: ['c.js'],
version: 3
};
const bSource = new SourceFile(_('/foo/src/b.js'), 'abcdef', bSourceMap, false, [null]);
const bSource =
new SourceFile(_('/foo/src/b.js'), 'abcdef', bSourceMap, false, [null], fs);
const aSourceMap: RawSourceMap = {
mappings: encode([[[0, 0, 0, 0], [2, 0, 0, 3], [4, 0, 0, 2], [5, 0, 0, 5]]]),
names: [],
sources: ['b.js'],
version: 3
};
const aSource =
new SourceFile(_('/foo/src/a.js'), 'abdecf', aSourceMap, false, [bSource]);
new SourceFile(_('/foo/src/a.js'), 'abdecf', aSourceMap, false, [bSource], fs);

// These flattened mappings are just the mappings from a to b.
// (The mappings to c are dropped since there is no source file to map to.)
Expand All @@ -462,23 +467,23 @@ runInEachFileSystem(() => {

describe('renderFlattenedSourceMap()', () => {
it('should convert the flattenedMappings into a raw source-map object', () => {
const cSource = new SourceFile(_('/foo/src/c.js'), 'bcd123e', null, false, []);
const cSource = new SourceFile(_('/foo/src/c.js'), 'bcd123e', null, false, [], fs);
const bToCSourceMap: RawSourceMap = {
mappings: encode([[[1, 0, 0, 0], [4, 0, 0, 3], [4, 0, 0, 6], [5, 0, 0, 7]]]),
names: [],
sources: ['c.js'],
version: 3
};
const bSource =
new SourceFile(_('/foo/src/b.js'), 'abcdef', bToCSourceMap, false, [cSource]);
new SourceFile(_('/foo/src/b.js'), 'abcdef', bToCSourceMap, false, [cSource], fs);
const aToBSourceMap: RawSourceMap = {
mappings: encode([[[0, 0, 0, 0], [2, 0, 0, 3], [4, 0, 0, 2], [5, 0, 0, 5]]]),
names: [],
sources: ['b.js'],
version: 3
};
const aSource =
new SourceFile(_('/foo/src/a.js'), 'abdecf', aToBSourceMap, false, [bSource]);
new SourceFile(_('/foo/src/a.js'), 'abdecf', aToBSourceMap, false, [bSource], fs);

const aTocSourceMap = aSource.renderFlattenedSourceMap();
expect(aTocSourceMap.version).toEqual(3);
Expand All @@ -493,7 +498,7 @@ runInEachFileSystem(() => {
});

it('should handle mappings that map from lines outside of the actual content lines', () => {
const bSource = new SourceFile(_('/foo/src/b.js'), 'abcdef', null, false, []);
const bSource = new SourceFile(_('/foo/src/b.js'), 'abcdef', null, false, [], fs);
const aToBSourceMap: RawSourceMap = {
mappings: encode([
[[0, 0, 0, 0], [2, 0, 0, 3], [4, 0, 0, 2], [5, 0, 0, 5]],
Expand All @@ -506,7 +511,7 @@ runInEachFileSystem(() => {
version: 3
};
const aSource =
new SourceFile(_('/foo/src/a.js'), 'abdecf', aToBSourceMap, false, [bSource]);
new SourceFile(_('/foo/src/a.js'), 'abdecf', aToBSourceMap, false, [bSource], fs);

const aTocSourceMap = aSource.renderFlattenedSourceMap();
expect(aTocSourceMap.version).toEqual(3);
Expand All @@ -522,13 +527,13 @@ runInEachFileSystem(() => {
describe('getOriginalLocation()', () => {
it('should return null for source files with no flattened mappings', () => {
const sourceFile =
new SourceFile(_('/foo/src/index.js'), 'index contents', null, false, []);
new SourceFile(_('/foo/src/index.js'), 'index contents', null, false, [], fs);
expect(sourceFile.getOriginalLocation(1, 1)).toEqual(null);
});

it('should return offset locations in multiple flattened original source files', () => {
const cSource = new SourceFile(_('/foo/src/c.js'), 'bcd123', null, false, []);
const dSource = new SourceFile(_('/foo/src/d.js'), 'aef', null, false, []);
const cSource = new SourceFile(_('/foo/src/c.js'), 'bcd123', null, false, [], fs);
const dSource = new SourceFile(_('/foo/src/d.js'), 'aef', null, false, [], fs);

const bSourceMap: RawSourceMap = {
mappings: encode([
Expand All @@ -542,8 +547,8 @@ runInEachFileSystem(() => {
sources: ['c.js', 'd.js'],
version: 3
};
const bSource =
new SourceFile(_('/foo/src/b.js'), 'abcdef', bSourceMap, false, [cSource, dSource]);
const bSource = new SourceFile(
_('/foo/src/b.js'), 'abcdef', bSourceMap, false, [cSource, dSource], fs);

const aSourceMap: RawSourceMap = {
mappings: encode([
Expand All @@ -560,7 +565,7 @@ runInEachFileSystem(() => {
version: 3
};
const aSource =
new SourceFile(_('/foo/src/a.js'), 'abde\n cf', aSourceMap, false, [bSource]);
new SourceFile(_('/foo/src/a.js'), 'abde\n cf', aSourceMap, false, [bSource], fs);

// Line 0
expect(aSource.getOriginalLocation(0, 0)) // a
Expand Down Expand Up @@ -592,8 +597,8 @@ runInEachFileSystem(() => {
});

it('should return offset locations across multiple lines', () => {
const originalSource =
new SourceFile(_('/foo/src/original.js'), 'abcdef\nghijk\nlmnop', null, false, []);
const originalSource = new SourceFile(
_('/foo/src/original.js'), 'abcdef\nghijk\nlmnop', null, false, [], fs);
const generatedSourceMap: RawSourceMap = {
mappings: encode([
[
Expand All @@ -614,7 +619,7 @@ runInEachFileSystem(() => {
};
const generatedSource = new SourceFile(
_('/foo/src/generated.js'), 'ABC\nGHIJDEFK\nLMNOP', generatedSourceMap, false,
[originalSource]);
[originalSource], fs);

// Line 0
expect(generatedSource.getOriginalLocation(0, 0)) // A
Expand Down
2 changes: 2 additions & 0 deletions packages/localize/src/tools/src/diagnostics.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ export type DiagnosticHandlingStrategy = 'error'|'warning'|'ignore';
/**
* This class is used to collect and then report warnings and errors that occur during the execution
* of the tools.
*
* @publicApi used by CLI
*/
export class Diagnostics {
readonly messages: {type: 'warning'|'error', message: string}[] = [];
Expand Down
6 changes: 4 additions & 2 deletions packages/localize/src/tools/src/extract/extraction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ export interface ExtractionOptions {
/**
* Extracts parsed messages from file contents, by parsing the contents as JavaScript
* and looking for occurrences of `$localize` in the source code.
*
* @publicApi used by CLI
*/
export class MessageExtractor {
private basePath: AbsoluteFsPath;
Expand Down Expand Up @@ -50,8 +52,8 @@ export class MessageExtractor {
sourceRoot: this.basePath,
filename,
plugins: [
makeEs2015ExtractPlugin(messages, this.localizeName),
makeEs5ExtractPlugin(messages, this.localizeName),
makeEs2015ExtractPlugin(this.fs, messages, this.localizeName),
makeEs5ExtractPlugin(this.fs, messages, this.localizeName),
],
code: false,
ast: false
Expand Down