Skip to content

Commit 0b5ff36

Browse files
hanslclydin
hansl
authored and
clydin
committed
fix(@ngtools/webpack): fix resolution fallback of paths-plugin
If there is a package.json we should also verify that it has a main or fesm field to see if webpack would actually resolve it properly. Otherwise use the JavaScript resolution. This is a temporary fix and still has obvious limitations and issues. Namely, this code is never run if there is only one path mapping, but that falls outside the scope of this fix. Also, some people might have valid packages but want to resolve to the JS file itself (which is what TypeScript does by default). These should be fixed with a refactoring of the path plugin. Fixes NativeScript/nativescript-dev-webpack#607
1 parent 4440a7a commit 0b5ff36

File tree

14 files changed

+227
-8
lines changed

14 files changed

+227
-8
lines changed

Diff for: ‎packages/ngtools/webpack/src/paths-plugin.ts

+33-8
Original file line numberDiff line numberDiff line change
@@ -160,15 +160,40 @@ export function resolveWithPaths(
160160
const jsFilePath = `${pathNoExtension}.js`;
161161

162162
if (host.fileExists(pathNoExtension)) {
163-
// This is mainly for secondary entry points
164-
// ex: 'node_modules/@angular/core/testing.d.ts' -> 'node_modules/@angular/core/testing'
165-
request.request = pathNoExtension;
166-
} else if (host.fileExists(packageRootPath)) {
167-
// Let webpack resolve the correct module format
168-
request.request = pathDirName;
169-
} else if (host.fileExists(jsFilePath)) {
163+
// This is mainly for secondary entry points
164+
// ex: 'node_modules/@angular/core/testing.d.ts' -> 'node_modules/@angular/core/testing'
165+
request.request = pathNoExtension;
166+
} else {
167+
const packageJsonContent = host.readFile(packageRootPath);
168+
let newRequest: string | undefined;
169+
170+
if (packageJsonContent) {
171+
try {
172+
const packageJson = JSON.parse(packageJsonContent);
173+
174+
// Let webpack resolve the correct module format IIF there is a module resolution field
175+
// in the package.json. These are all official fields that Angular uses.
176+
if (typeof packageJson.main == 'string'
177+
|| typeof packageJson.browser == 'string'
178+
|| typeof packageJson.module == 'string'
179+
|| typeof packageJson.es2015 == 'string'
180+
|| typeof packageJson.fesm5 == 'string'
181+
|| typeof packageJson.fesm2015 == 'string') {
182+
newRequest = pathDirName;
183+
}
184+
} catch {
185+
// Ignore exceptions and let it fall through (ie. if package.json file is invalid).
186+
}
187+
}
188+
189+
if (newRequest === undefined && host.fileExists(jsFilePath)) {
170190
// Otherwise, if there is a file with a .js extension use that
171-
request.request = jsFilePath;
191+
newRequest = jsFilePath;
192+
}
193+
194+
if (newRequest !== undefined) {
195+
request.request = newRequest;
196+
}
172197
}
173198

174199
callback(null, request);

Diff for: ‎tests/legacy-cli/e2e/assets/webpack/test-app-path-mapping/aotplugin.config.json

+4
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
{
2+
"tsConfigPath": "./src/tsconfig.json",
3+
"mainPath": "app/main.jit.ts"
4+
}

Diff for: ‎tests/legacy-cli/e2e/assets/webpack/test-app-path-mapping/package.json

+29
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
{
2+
"name": "test",
3+
"license": "MIT",
4+
"dependencies": {
5+
"@angular/common": "^5.0.0-rc.8",
6+
"@angular/compiler": "^5.0.0-rc.8",
7+
"@angular/compiler-cli": "^5.0.0-rc.8",
8+
"@angular/core": "^5.0.0-rc.8",
9+
"@angular/http": "^5.0.0-rc.8",
10+
"@angular/platform-browser": "^5.0.0-rc.8",
11+
"@angular/platform-browser-dynamic": "^5.0.0-rc.8",
12+
"@angular/platform-server": "^5.0.0-rc.8",
13+
"@angular/router": "^5.0.0-rc.8",
14+
"@ngtools/webpack": "0.0.0",
15+
"core-js": "^2.4.1",
16+
"rxjs": "^5.5.0",
17+
"zone.js": "^0.8.14"
18+
},
19+
"devDependencies": {
20+
"node-sass": "^4.7.0",
21+
"performance-now": "^0.2.0",
22+
"preprocess-loader": "^0.2.2",
23+
"raw-loader": "^0.5.1",
24+
"sass-loader": "^6.0.0",
25+
"typescript": "~2.4.2",
26+
"webpack": "~4.0.1",
27+
"webpack-cli": "~2.0.9"
28+
}
29+
}

Diff for: ‎tests/legacy-cli/e2e/assets/webpack/test-app-path-mapping/src/app/app.module.ts

+29
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
/**
2+
* @license
3+
* Copyright Google Inc. All Rights Reserved.
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.io/license
7+
*/
8+
import { Component, NgModule } from '@angular/core';
9+
import { BrowserModule } from '@angular/platform-browser';
10+
import { Foo } from 'foo/foo';
11+
12+
console.log(Foo); // Make sure it's used.
13+
14+
@Component({
15+
selector: 'home-view',
16+
template: 'home!',
17+
})
18+
export class HomeView {}
19+
20+
@NgModule({
21+
declarations: [
22+
HomeView,
23+
],
24+
imports: [
25+
BrowserModule,
26+
],
27+
bootstrap: [HomeView],
28+
})
29+
export class AppModule { }

Diff for: ‎tests/legacy-cli/e2e/assets/webpack/test-app-path-mapping/src/app/main.jit.ts

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
import 'core-js/es7/reflect';
2+
import {platformBrowserDynamic} from '@angular/platform-browser-dynamic';
3+
import {AppModule} from './app.module';
4+
5+
platformBrowserDynamic().bootstrapModule(AppModule);

Diff for: ‎tests/legacy-cli/e2e/assets/webpack/test-app-path-mapping/src/foo/foo.d.ts

+9
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
/**
2+
* @license
3+
* Copyright Google Inc. All Rights Reserved.
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.io/license
7+
*/
8+
9+
export class Foo {}

Diff for: ‎tests/legacy-cli/e2e/assets/webpack/test-app-path-mapping/src/foo/foo.js

+6
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
2+
console.log('NGTOOLS_WEBPACK_TEST_WRONG_FILE');
3+
4+
module.exports = {
5+
Foo: () => {}, // Empty "class".
6+
};

Diff for: ‎tests/legacy-cli/e2e/assets/webpack/test-app-path-mapping/src/foo/package.json

+3
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
{
2+
"main": "right/foo.js"
3+
}

Diff for: ‎tests/legacy-cli/e2e/assets/webpack/test-app-path-mapping/src/foo/right/foo.js

+6
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
2+
console.log('NGTOOLS_WEBPACK_TEST_RIGHT_FILE');
3+
4+
module.exports = {
5+
Foo: () => {}, // Empty "class".
6+
};

Diff for: ‎tests/legacy-cli/e2e/assets/webpack/test-app-path-mapping/src/index.html

+12
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
<html>
2+
<head>
3+
<meta charset="UTF-8">
4+
<title>Document</title>
5+
<base href="">
6+
</head>
7+
<body>
8+
<app-root></app-root>
9+
<script src="node_modules/zone.js/dist/zone.js"></script>
10+
<script src="dist/app.main.js"></script>
11+
</body>
12+
</html>

Diff for: ‎tests/legacy-cli/e2e/assets/webpack/test-app-path-mapping/src/tsconfig.json

+30
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
{
2+
"compilerOptions": {
3+
"module": "es2015",
4+
"moduleResolution": "node",
5+
"target": "es5",
6+
"baseUrl": ".",
7+
"noImplicitAny": false,
8+
"sourceMap": true,
9+
"mapRoot": "",
10+
"emitDecoratorMetadata": true,
11+
"experimentalDecorators": true,
12+
"lib": [
13+
"es2017",
14+
"dom"
15+
],
16+
"outDir": "lib",
17+
"skipLibCheck": true,
18+
"rootDir": ".",
19+
"paths": {
20+
"foo/*": [
21+
"./foo/*",
22+
"*"
23+
]
24+
}
25+
},
26+
"angularCompilerOptions": {
27+
"genDir": "app/generated/",
28+
"entryModule": "app/app.module#AppModule"
29+
}
30+
}

Diff for: ‎tests/legacy-cli/e2e/assets/webpack/test-app-path-mapping/webpack.config.js

+45
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
/**
2+
* @license
3+
* Copyright Google Inc. All Rights Reserved.
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.io/license
7+
*/
8+
const ngToolsWebpack = require('@ngtools/webpack');
9+
const path = require('path');
10+
11+
const flags = require('./webpack.flags.json');
12+
13+
const preprocessLoader = 'preprocess-loader' + (flags.DEBUG ? '?+DEBUG' : '');
14+
15+
16+
module.exports = {
17+
resolve: {
18+
extensions: ['.ts', '.js']
19+
},
20+
entry: './src/app/main.jit.ts',
21+
output: {
22+
path: path.resolve('./dist'),
23+
publicPath: 'dist/',
24+
filename: 'app.main.js'
25+
},
26+
plugins: [
27+
new ngToolsWebpack.AngularCompilerPlugin(require('./aotplugin.config.json'))
28+
],
29+
module: {
30+
rules: [
31+
{ test: /\.scss$/, loaders: ['raw-loader', 'sass-loader', preprocessLoader] },
32+
{ test: /\.css$/, loader: 'raw-loader' },
33+
{ test: /\.html$/, loaders: ['raw-loader', preprocessLoader] },
34+
// Use preprocess to remove DEBUG only code.
35+
// @ngtools/webpack must be the first (right most) loader.
36+
{ test: /\.ts$/, use: [
37+
{ loader: preprocessLoader },
38+
{ loader: '@ngtools/webpack' }
39+
] }
40+
]
41+
},
42+
devServer: {
43+
historyApiFallback: true
44+
}
45+
};

Diff for: ‎tests/legacy-cli/e2e/assets/webpack/test-app-path-mapping/webpack.flags.json

+3
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
{
2+
"DEBUG": false
3+
}

Diff for: ‎tests/legacy-cli/e2e/tests/packages/webpack/test-path-mapping.ts

+13
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
import {normalize} from 'path';
2+
import {createProjectFromAsset} from '../../../utils/assets';
3+
import {exec} from '../../../utils/process';
4+
import {expectFileSizeToBeUnder, replaceInFile, expectFileToMatch} from '../../../utils/fs';
5+
6+
7+
export default function(skipCleaning: () => void) {
8+
return Promise.resolve()
9+
.then(() => createProjectFromAsset('webpack/test-app-path-mapping'))
10+
.then(() => exec(normalize('node_modules/.bin/webpack-cli')))
11+
.then(() => expectFileToMatch('dist/app.main.js', 'NGTOOLS_WEBPACK_TEST_RIGHT_FILE'))
12+
.then(() => skipCleaning());
13+
}

0 commit comments

Comments
 (0)