Skip to content

fix(@schematics/angular): generate tsconfig.worker.json outside of … #14188

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

Merged
merged 1 commit into from
Apr 18, 2019
Merged

fix(@schematics/angular): generate tsconfig.worker.json outside of … #14188

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/angular_devkit/build_angular/test/browser/web-worker_spec_large.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ describe('Browser Builder Web Worker support', () => {
const workerFiles: { [k: string]: string } = {
'src/app/dep.ts': `export const foo = 'bar';`,
'src/app/app.worker.ts': `
/// <reference lib="webworker" />
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note: this is solely used for IDE purposes.


import { foo } from './dep';
console.log('hello from worker');
addEventListener('message', ({ data }) => {
Expand Down
10 changes: 0 additions & 10 deletions packages/schematics/angular/web-worker/files/project-tsconfig/tsconfig.json.template
View file
Open in desktop

This file was deleted.

4 changes: 2 additions & 2 deletions packages/schematics/angular/web-worker/files/worker-tsconfig/tsconfig.worker.json.template
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"extends": "./tsconfig.json",
"extends": "<%= relativePathToWorkspaceRoot %>/tsconfig.json",
"compilerOptions": {
"outDir": "<%= relativePathToWorkspaceRoot %>/out-tsc/worker",
"lib": [
Expand All @@ -9,6 +9,6 @@
"types": []
},
"include": [
"**/*.worker.ts"
"src/**/*.worker.ts"
]
}
2 changes: 2 additions & 0 deletions packages/schematics/angular/web-worker/files/worker/__name@dasherize__.worker.ts.template
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
/// <reference lib="webworker" />

addEventListener('message', ({ data }) => {
const response = `worker response to ${data}`;
postMessage(response);
Expand Down
74 changes: 19 additions & 55 deletions packages/schematics/angular/web-worker/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/
import { JsonParseMode, parseJsonAst, strings, tags } from '@angular-devkit/core';
import { JsonParseMode, join, normalize, parseJsonAst, strings, tags } from '@angular-devkit/core';
import {
Rule, SchematicContext, SchematicsException, Tree,
apply, applyTemplates, chain, mergeWith, move, noop, url,
Expand All @@ -21,54 +21,16 @@ function addConfig(options: WebWorkerOptions, root: string, tsConfigPath: string
return (host: Tree, context: SchematicContext) => {
context.logger.debug('updating project configuration.');

const tsConfigRules = [];

// Add tsconfig.worker.json.
const relativePathToWorkspaceRoot = root.split('/').map(x => '..').join('/');
tsConfigRules.push(mergeWith(apply(url('./files/worker-tsconfig'), [
applyTemplates({ ...options, relativePathToWorkspaceRoot }),
move(root),
])));

// Add project tsconfig.json.
// The project level tsconfig.json with webworker lib is for editor support since
// the dom and webworker libs are mutually exclusive.
// Note: this schematic does not change other tsconfigs to use the project-level tsconfig.
const projectTsConfigPath = `${root}/tsconfig.json`;
if (host.exists(projectTsConfigPath)) {
// If the file already exists, alter it.
const buffer = host.read(projectTsConfigPath);
if (buffer) {
const tsCfgAst = parseJsonAst(buffer.toString(), JsonParseMode.Loose);
if (tsCfgAst.kind != 'object') {
throw new SchematicsException('Invalid tsconfig. Was expecting an object');
}
const optsAstNode = findPropertyInAstObject(tsCfgAst, 'compilerOptions');
if (optsAstNode && optsAstNode.kind != 'object') {
throw new SchematicsException(
'Invalid tsconfig "compilerOptions" property; Was expecting an object.');
}
const libAstNode = findPropertyInAstObject(tsCfgAst, 'lib');
if (libAstNode && libAstNode.kind != 'array') {
throw new SchematicsException('Invalid tsconfig "lib" property; expected an array.');
}
const newLibProp = 'webworker';
if (libAstNode && !libAstNode.value.includes(newLibProp)) {
const recorder = host.beginUpdate(projectTsConfigPath);
appendValueInAstArray(recorder, libAstNode, newLibProp);
host.commitUpdate(recorder);
}
}
} else {
// Otherwise create it.
tsConfigRules.push(mergeWith(apply(url('./files/project-tsconfig'), [
applyTemplates({ ...options, relativePathToWorkspaceRoot }),
move(root),
])));
}
// todo: replace with the new helper method in a seperate PR
// https://github.com/angular/angular-cli/pull/14207
const rootNormalized = root.endsWith('/') ? root.slice(0, -1) : root;
const relativePathToWorkspaceRoot =
rootNormalized
? rootNormalized.split('/').map(x => '..').join('/')
: '.';

// Add worker glob exclusion to tsconfig.app.json.
const workerGlob = '**/*.worker.ts';
const workerGlob = 'src/**/*.worker.ts';
const buffer = host.read(tsConfigPath);
if (buffer) {
const tsCfgAst = parseJsonAst(buffer.toString(), JsonParseMode.Loose);
Expand All @@ -80,17 +42,19 @@ function addConfig(options: WebWorkerOptions, root: string, tsConfigPath: string
throw new SchematicsException('Invalid tsconfig "exclude" property; expected an array.');
}

if (filesAstNode && filesAstNode.value.indexOf(workerGlob) == -1) {
if (filesAstNode && !filesAstNode.value.includes(workerGlob)) {
const recorder = host.beginUpdate(tsConfigPath);
appendValueInAstArray(recorder, filesAstNode, workerGlob);
host.commitUpdate(recorder);
}
}

return chain([
// Add tsconfigs.
...tsConfigRules,
]);
return mergeWith(
apply(url('./files/worker-tsconfig'), [
applyTemplates({ ...options, relativePathToWorkspaceRoot }),
move(root),
]),
);
};
}

Expand Down Expand Up @@ -145,7 +109,7 @@ export default function (options: WebWorkerOptions): Rule {
throw new SchematicsException('Option "project" is required.');
}
if (!options.target) {
throw new SchematicsException('Option (target) is required.');
throw new SchematicsException('Option "target" is required.');
}

const project = workspace.projects.get(options.project);
Expand All @@ -169,11 +133,11 @@ export default function (options: WebWorkerOptions): Rule {
const parsedPath = parseName(options.path, options.name);
options.name = parsedPath.name;
options.path = parsedPath.path;
const root = project.root || project.sourceRoot || '';
const root = project.root || '';
Copy link
Member

@gkalpak gkalpak Apr 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

AFAICT, this raises the following concerns (maybe they are not problems - I am not sure):

  1. The exclude path in tsconfig.json should also be updated to include src/ (**/*.worker.ts --> src/**/*.worker.ts).
  2. By updating the main tsconfig.json with lib: [..., 'webworker'] (i.e. here or there), this lib is now applied to all configs which inherit from the main onet (e.g. tsconfig.app.json, tsconfig.spec.json, e2e/tsconfig.json).
    Is this intentional (or should other configs be updated to overwrite lib and not include webworker)?

Copy link
Collaborator Author

@alan-agius4 alan-agius4 Apr 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

While the first point itself is not a problem per see, the second one might be.

Also this might be confusing for users to have multiple tsconfigs though I cannot think of an alternative and place the second tsconfig in the src folder.

Ie:

-- tsconfig.json
-- tsconfig.worker.json
-- tsconfig.app.json
-- tsconfig.spec.json
-- src
---- tsconfig.json

@filipesilva, what are your thoughts about this? I don't think there is much we can do apart from creating the new tsconfig.json under src, and make a note in it stating that the config is solely used for IDEs

Copy link
Collaborator Author

@alan-agius4 alan-agius4 Apr 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Also it seems that this tsconfig https://github.com/alan-agius4/angular-cli/blob/c6315fee4121ed29c7e25496e692cf2cc66ff27e/packages/schematics/angular/web-worker/files/project-tsconfig/tsconfig.json.template has both DOM and webworker. I was under the impression that they are mutually exclusive.

Also this config has no include/exclude pattern which means that all Ts files within the project will be included.

Copy link
Collaborator Author

@alan-agius4 alan-agius4 Apr 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Also probably the worker tsconfig should inherit from the workspace tsconfig directly and not the ide tsconfig.

Copy link
Contributor

Choose a reason for hiding this comment

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

The general idea is that there is at least one tsconfig that is called just tsconfig.json and includes typings for both dom and webworker. Such configs are meant to be picked up by the IDE to provide accurate typings.

There are also at least three different known locations for app tsconfigs in the wild:

  • somefolder/projectname/tsconfig.app.json, created on multi-project workspaces
  • src/tsconfig.app.json, created by CLI 1,6,7
  • tsconfig.app.json, created by CLI 8

Also of note is that in CLI 8 the multi-project workspace story is "don't use your current one, but make a new one with ng new --create-application instead".

The new tsconfig.worker.json suffers from a similar problem as tsconfig.spec.json, in that files for its compilation are intersped with application files. This means we can't rely on a clear separation of TS compilation contexts by folder and must show typings for both in many files.

Compounding on this problem is that dom and webworker typings are incompatible (microsoft/TypeScript#20595) for compilation purposes. So any compilation unit that has them both will fail. This doesn't seem to be a problem for IDE support though.

So, back to the original questions:

  1. what base tsconfig.json should tsconfig.worker.json extend from
  2. what IDE tsconfig.json should provide the dom+webworker typings

I think the super correct answer to both 1 and 2 is the root /tsconfig.json. But this means altering that one to have both dom+webworker, and then altering all tsconfigs that inherit from it to only have either dom or webworker.

This is non trivial and high risk. We don't know what configs inherit from the base one and missing any of them will cause that compilation to fail. Also when making new projects after this change, the new project would also need to know that the base tsconfig has incompatible typings and perform the same change.

So the less risky approach is for 1 to be the root /tsconfig.json, and 2 to be a new src/tsconfig.json that we create just for IDE support, and from which nothing inherits. I like this option better because it doesn't break anyone, and is more future proof. Users can just drop the IDE one if the incompatible typings situation changes in the future, or if IDEs start supporting the differently names tsconfigs. But it adds another file and we want to keep the count low.

Another option is for 1 to be the root /tsconfig.json, and instead of doing 2 we have some hard cast in the .worker.ts file similar to the approach described in microsoft/TypeScript#20595 (comment). I tried a bunch of these and couldn't get a good one. But given that the other approaches have significant downsides maybe it's worth looking further into.

Copy link
Collaborator Author

@alan-agius4 alan-agius4 Apr 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

While for the compilation we do need a separate tsconfig as otherwise we'll get compilation errors like

node_modules/typescript/lib/lib.webworker.d.ts:4344:13 - error TS2403: Subsequent variable declarations must have the same type.  Variable 'navigator' must be of type 'Navigator', but here has type 'WorkerNavigator'.

4344 declare var navigator: WorkerNavigator;

For IDE support, we can use the triple slash lib reference /// <reference lib="webworker" /> in the web worker file, though this doesn't doesn't fix the issue when the same symbols are defined in both DOM and WebWorker libs (Which is already present with the dud tsconfig, since the later has both dom and webworker as libs).

Such an example is self

Writing something like:

const _self = self;

Would result in _self being of type Window and not WorkerGlobalScope.

And hence to have proper typings one needs to explicitly set the type like:

const _self: WorkerGlobalScope = self;

This is because of what @filipesilva explained above, and the only way I can think to solve this, is that workers, are not created inside the features folders, but rather in a separate folder similar to e2e.

Ie:

-- tsconfig.json
-- tsconfig.app.json
-- tsconfig.spec.json
-- src
---- workers
------ feature-foo.worker.ts
------ tsconfig.json
---- feature-foo
------ feature-foo.component.ts


const needWebWorkerConfig = !projectTargetOptions.webWorkerTsConfig;
if (needWebWorkerConfig) {
const workerConfigPath = `${root.endsWith('/') ? root : root + '/'}tsconfig.worker.json`;
const workerConfigPath = join(normalize(root), 'tsconfig.worker.json');
Copy link
Member

Choose a reason for hiding this comment

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

Won't this result in a normalized absolute path? If so, is that intended?
(Or is project.root relative?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Project root is relative 😊

projectTargetOptions.webWorkerTsConfig = workerConfigPath;

// add worker tsconfig to lint architect target
Expand Down
29 changes: 20 additions & 9 deletions packages/schematics/angular/web-worker/index_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ describe('Service Worker Schematic', () => {
project: 'bar',
target: 'build',
name: 'app',
// path: 'src/app',
snippet: true,
};

Expand Down Expand Up @@ -54,18 +53,14 @@ describe('Service Worker Schematic', () => {
expect(tree.exists(path)).toEqual(true);
});

it('should put a new tsconfig.json file in the project root', async () => {
const tree = await schematicRunner.runSchematicAsync('web-worker', defaultOptions, appTree)
.toPromise();
const path = '/projects/bar/tsconfig.json';
expect(tree.exists(path)).toEqual(true);
});

it('should put the tsconfig.worker.json file in the project root', async () => {
const tree = await schematicRunner.runSchematicAsync('web-worker', defaultOptions, appTree)
.toPromise();
const path = '/projects/bar/tsconfig.worker.json';
expect(tree.exists(path)).toEqual(true);

const { compilerOptions } = JSON.parse(tree.readContent(path));
expect(compilerOptions.outDir).toBe('../../out-tsc/worker');
});

it('should add the webWorkerTsConfig option to workspace', async () => {
Expand All @@ -80,7 +75,7 @@ describe('Service Worker Schematic', () => {
const tree = await schematicRunner.runSchematicAsync('web-worker', defaultOptions, appTree)
.toPromise();
const { exclude } = JSON.parse(tree.readContent('/projects/bar/tsconfig.app.json'));
expect(exclude).toContain('**/*.worker.ts');
expect(exclude).toContain('src/**/*.worker.ts');
});

it('should add snippet to sibling file', async () => {
Expand All @@ -103,4 +98,20 @@ describe('Service Worker Schematic', () => {
'projects/bar/tsconfig.worker.json',
]);
});

it(`should add 'tsconfig.worker.json' outside of 'src' directory in root app`, async () => {
const rootAppOptions = { ...appOptions, projectRoot: '', name: 'foo' };
const workerOptions = { ...defaultOptions, project: 'foo' };

appTree = await schematicRunner.runSchematicAsync('application', rootAppOptions, appTree)
.toPromise();
const tree = await schematicRunner.runSchematicAsync('web-worker', workerOptions, appTree)
.toPromise();

const path = '/tsconfig.worker.json';
expect(tree.exists(path)).toEqual(true);

const { compilerOptions } = JSON.parse(tree.readContent(path));
expect(compilerOptions.outDir).toBe('./out-tsc/worker');
});
});
4 changes: 2 additions & 2 deletions tests/legacy-cli/e2e/tests/build/worker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ export default async function () {

const workerPath = join('src', 'app', 'app.worker.ts');
const snippetPath = join('src', 'app', 'app.component.ts');
const projectTsConfig = join('src', 'tsconfig.json');
const workerTsConfig = join('src', 'tsconfig.worker.json');
const projectTsConfig = 'tsconfig.json';
const workerTsConfig = 'tsconfig.worker.json';

await ng('generate', 'web-worker', 'app');
await expectFileToExist(workerPath);
Expand Down