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.

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

merged 1 commit into from
Apr 18, 2019

Conversation

alan-agius4
Copy link
Collaborator

@alan-agius4 alan-agius4 commented Apr 16, 2019

…the src folder

This is to align with the folder structure of version 8, were tsconfigs are outside of the src folder

Also, this change remove the dud tsconfig.json in the src folder and instead we add the triple slash lib reference /// <reference lib="webworker" /> for IDE support.

@alan-agius4 alan-agius4 added the target: major This PR is targeted for the next major release label Apr 16, 2019
@alan-agius4
Copy link
Collaborator Author

//cc @gkalpak

Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

🎉

@@ -169,11 +174,12 @@ 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

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

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

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

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

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

@@ -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.

…the `src` folder

This is to align with the folder structure of version 8, were tsconfigs are outside of the `src` folder

Also, this change remove the dud `tsconfig.json` in the `src` folder and instead we add the triple slash lib reference `/// <reference lib="webworker" />` for IDE support.
@alan-agius4 alan-agius4 added the needs: discussion On the agenda for team meeting to determine next steps label Apr 18, 2019
@mgechev mgechev removed the needs: discussion On the agenda for team meeting to determine next steps label Apr 18, 2019
@alan-agius4 alan-agius4 requested a review from gkalpak April 18, 2019 17:31
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

One question. Otherwise LGTM 👍


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 😊

@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 Sep 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants