Skip to content

Commit 47c4ccb

Browse files
alicewriteswrongsalicewriteswrongs
authoredJul 21, 2022
fix(compiler): update package.json validation for the 'module' field (#3475)
This updates the validation around the `module` field in `package.json` to take the configured output targets into account. In particular, for `DIST_CUSTOM_ELEMENTS_BUNDLE` it should be something like `dist/index.js`, whereas for `DIST_CUSTOM_ELEMENTS` it should be something like `dist/components/index.js`.
·
v4.29.0v2-maintenance
1 parent 65f5275 commit 47c4ccb

File tree

3 files changed

+254
-49
lines changed

3 files changed

+254
-49
lines changed
 

‎src/compiler/types/tests/validate-package-json.spec.ts

+72-10
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,8 @@ import type * as d from '@stencil/core/declarations';
22
import { mockBuildCtx, mockCompilerCtx, mockConfig } from '@stencil/core/testing';
33
import * as v from '../validate-build-package-json';
44
import path from 'path';
5-
import { DIST_CUSTOM_ELEMENTS_BUNDLE } from '../../output-targets/output-utils';
5+
import { DIST_COLLECTION, DIST_CUSTOM_ELEMENTS, DIST_CUSTOM_ELEMENTS_BUNDLE } from '../../output-targets/output-utils';
6+
import { normalizePath } from '../../../utils/normalize-path';
67

78
describe('validate-package-json', () => {
89
let config: d.Config;
@@ -84,11 +85,11 @@ describe('validate-package-json', () => {
8485
config.outputTargets = [];
8586
compilerCtx.fs.writeFile(path.join(root, 'dist', 'index.js'), '');
8687
buildCtx.packageJson.module = 'dist/index.js';
87-
v.validateModule(config, compilerCtx, buildCtx, collectionOutputTarget);
88+
await v.validateModule(config, compilerCtx, buildCtx);
8889
expect(buildCtx.diagnostics).toHaveLength(0);
8990
});
9091

91-
it('validate custom elements module', async () => {
92+
it('validate custom elements bundle module', async () => {
9293
config.outputTargets = [
9394
{
9495
type: DIST_CUSTOM_ELEMENTS_BUNDLE,
@@ -97,25 +98,86 @@ describe('validate-package-json', () => {
9798
];
9899
compilerCtx.fs.writeFile(path.join(root, 'dist', 'index.js'), '');
99100
buildCtx.packageJson.module = 'custom-elements/index.js';
100-
v.validateModule(config, compilerCtx, buildCtx, collectionOutputTarget);
101+
await v.validateModule(config, compilerCtx, buildCtx);
101102
expect(buildCtx.diagnostics).toHaveLength(0);
102103
});
103104

105+
it('validates a valid custom elements module', async () => {
106+
config.outputTargets = [
107+
{
108+
type: DIST_CUSTOM_ELEMENTS,
109+
dir: path.join(root, 'dist'),
110+
},
111+
];
112+
buildCtx.packageJson.module = 'dist/components/index.js';
113+
await v.validateModule(config, compilerCtx, buildCtx);
114+
expect(buildCtx.diagnostics).toHaveLength(0);
115+
});
116+
117+
it('errors on an invalid custom elements module', async () => {
118+
config.outputTargets = [
119+
{
120+
type: DIST_CUSTOM_ELEMENTS,
121+
dir: path.join(root, 'dist'),
122+
},
123+
];
124+
buildCtx.packageJson.module = 'dist/index.js';
125+
await v.validateModule(config, compilerCtx, buildCtx);
126+
expect(buildCtx.diagnostics).toHaveLength(1);
127+
const [diagnostic] = buildCtx.diagnostics;
128+
expect(diagnostic.level).toBe('warn');
129+
expect(diagnostic.messageText).toBe(
130+
`package.json "module" property is set to "dist/index.js". It's recommended to set the "module" property to: ./dist/components/index.js`
131+
);
132+
});
133+
104134
it('missing dist module', async () => {
105135
config.outputTargets = [];
106-
v.validateModule(config, compilerCtx, buildCtx, collectionOutputTarget);
136+
await v.validateModule(config, compilerCtx, buildCtx);
107137
expect(buildCtx.diagnostics).toHaveLength(1);
138+
const [diagnostic] = buildCtx.diagnostics;
139+
expect(diagnostic.level).toBe('warn');
140+
expect(diagnostic.messageText).toBe('package.json "module" property is required when generating a distribution.');
108141
});
109142

110-
it('missing dist module, but has custom elements output', async () => {
111-
config.outputTargets = [
112-
{
143+
it.each<{
144+
ot: d.OutputTarget;
145+
path: string;
146+
}>([
147+
{
148+
ot: {
113149
type: DIST_CUSTOM_ELEMENTS_BUNDLE,
114150
dir: path.join(root, 'custom-elements'),
115151
},
116-
];
117-
v.validateModule(config, compilerCtx, buildCtx, collectionOutputTarget);
152+
path: 'custom-elements/index.js',
153+
},
154+
{
155+
ot: {
156+
type: DIST_CUSTOM_ELEMENTS,
157+
dir: path.join(root, 'dist'),
158+
},
159+
path: 'dist/components/index.js',
160+
},
161+
{
162+
ot: {
163+
type: DIST_COLLECTION,
164+
dir: path.join(root, 'dist'),
165+
collectionDir: 'dist/collection',
166+
},
167+
path: 'dist/index.js',
168+
},
169+
])('errors on missing module w/ $ot.type, suggests $path', async ({ ot, path }) => {
170+
config.outputTargets = [ot];
171+
buildCtx.packageJson.module = undefined;
172+
await v.validateModule(config, compilerCtx, buildCtx);
118173
expect(buildCtx.diagnostics).toHaveLength(1);
174+
const [diagnostic] = buildCtx.diagnostics;
175+
expect(diagnostic.level).toBe('warn');
176+
expect(diagnostic.messageText).toBe(
177+
`package.json "module" property is required when generating a distribution. It's recommended to set the "module" property to: ${normalizePath(
178+
path
179+
)}`
180+
);
119181
});
120182
});
121183

‎src/compiler/types/validate-build-package-json.ts

+165-35
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,20 @@ import { dirname, join, relative } from 'path';
44
import {
55
getComponentsDtsTypesFilePath,
66
isOutputTargetDistCollection,
7+
isOutputTargetDistCustomElements,
78
isOutputTargetDistCustomElementsBundle,
89
isOutputTargetDistTypes,
910
} from '../output-targets/output-utils';
1011

12+
/**
13+
* Validate the package.json file for a project, checking that various fields
14+
* are set correctly for the currently-configured output targets.
15+
*
16+
* @param config the user-supplied Stencil config
17+
* @param compilerCtx the compiler context
18+
* @param buildCtx the build context
19+
* @returns an empty Promise
20+
*/
1121
export const validateBuildPackageJson = async (config: d.Config, compilerCtx: d.CompilerCtx, buildCtx: d.BuildCtx) => {
1222
if (config.watch) {
1323
return;
@@ -16,19 +26,28 @@ export const validateBuildPackageJson = async (config: d.Config, compilerCtx: d.
1626
return;
1727
}
1828

19-
const outputTargets = config.outputTargets.filter(isOutputTargetDistCollection);
29+
const distCollectionOutputTargets = config.outputTargets.filter(isOutputTargetDistCollection);
2030
const typesOutputTargets = config.outputTargets.filter(isOutputTargetDistTypes);
2131
await Promise.all([
22-
...outputTargets.map((outputsTarget) => {
23-
return validatePackageJsonOutput(config, compilerCtx, buildCtx, outputsTarget);
24-
}),
25-
...typesOutputTargets.map((outputTarget) => {
26-
return validateTypes(config, compilerCtx, buildCtx, outputTarget);
27-
}),
32+
...distCollectionOutputTargets.map((distCollectionOT) =>
33+
validateDistCollectionPkgJson(config, compilerCtx, buildCtx, distCollectionOT)
34+
),
35+
...typesOutputTargets.map((typesOT) => validateTypes(config, compilerCtx, buildCtx, typesOT)),
36+
validateModule(config, compilerCtx, buildCtx),
2837
]);
2938
};
3039

31-
const validatePackageJsonOutput = async (
40+
/**
41+
* Validate package.json contents for the `DIST_COLLECTION` output target,
42+
* checking that various fields like `files`, `main`, and so on are set
43+
* correctly.
44+
*
45+
* @param config the stencil config
46+
* @param compilerCtx the current compiler context
47+
* @param buildCtx the current build context
48+
* @param outputTarget a DIST_COLLECTION output target
49+
*/
50+
const validateDistCollectionPkgJson = async (
3251
config: d.Config,
3352
compilerCtx: d.CompilerCtx,
3453
buildCtx: d.BuildCtx,
@@ -37,12 +56,20 @@ const validatePackageJsonOutput = async (
3756
await Promise.all([
3857
validatePackageFiles(config, compilerCtx, buildCtx, outputTarget),
3958
validateMain(config, compilerCtx, buildCtx, outputTarget),
40-
validateModule(config, compilerCtx, buildCtx, outputTarget),
4159
validateCollection(config, compilerCtx, buildCtx, outputTarget),
4260
validateBrowser(config, compilerCtx, buildCtx),
4361
]);
4462
};
4563

64+
/**
65+
* Validate that the `files` field in `package.json` contains directories and
66+
* files that are necessary for the `DIST_COLLECTION` output target.
67+
*
68+
* @param config the stencil config
69+
* @param compilerCtx the current compiler context
70+
* @param buildCtx the current build context
71+
* @param outputTarget a DIST_COLLECTION output target
72+
*/
4673
export const validatePackageFiles = async (
4774
config: d.Config,
4875
compilerCtx: d.CompilerCtx,
@@ -81,6 +108,15 @@ export const validatePackageFiles = async (
81108
}
82109
};
83110

111+
/**
112+
* Check that the `main` field is set correctly in `package.json` for the
113+
* `DIST_COLLECTION` output target.
114+
*
115+
* @param config the stencil config
116+
* @param compilerCtx the current compiler context
117+
* @param buildCtx the current build context
118+
* @param outputTarget a DIST_COLLECTION output target
119+
*/
84120
export const validateMain = (
85121
config: d.Config,
86122
compilerCtx: d.CompilerCtx,
@@ -99,35 +135,88 @@ export const validateMain = (
99135
}
100136
};
101137

102-
export const validateModule = (
103-
config: d.Config,
104-
compilerCtx: d.CompilerCtx,
105-
buildCtx: d.BuildCtx,
106-
outputTarget: d.OutputTargetDistCollection
107-
) => {
108-
const customElementsOutput = config.outputTargets.find(isOutputTargetDistCustomElementsBundle);
138+
/**
139+
* Validate the package.json 'module' field, taking into account output targets
140+
* and other configuration details. This will look for a value for the `module`
141+
* field. If not present it will set a relevant warning message with an
142+
* output-target specific recommended value. If it is present and is not equal
143+
* to that recommended value it will set a different warning message.
144+
*
145+
* @param config the current user-supplied configuration
146+
* @param compilerCtx the compiler context
147+
* @param buildCtx the build context
148+
* @returns an empty Promise
149+
*/
150+
export const validateModule = async (config: d.Config, compilerCtx: d.CompilerCtx, buildCtx: d.BuildCtx) => {
109151
const currentModule = buildCtx.packageJson.module;
110-
const distAbs = join(outputTarget.dir, 'index.js');
111-
const distRel = relative(config.rootDir, distAbs);
112152

113-
let recommendedRelPath = distRel;
114-
if (customElementsOutput) {
115-
const customElementsAbs = join(customElementsOutput.dir, 'index.js');
116-
recommendedRelPath = relative(config.rootDir, customElementsAbs);
117-
}
153+
const recommendedRelPath = recommendedModulePath(config);
118154

119155
if (!isString(currentModule)) {
120-
const msg = `package.json "module" property is required when generating a distribution. It's recommended to set the "module" property to: ${recommendedRelPath}`;
156+
let msg = 'package.json "module" property is required when generating a distribution.';
157+
158+
if (recommendedRelPath !== null) {
159+
msg += ` It's recommended to set the "module" property to: ${normalizePath(recommendedRelPath)}`;
160+
}
121161
packageJsonWarn(config, compilerCtx, buildCtx, msg, `"module"`);
122-
} else if (
123-
normalizePath(currentModule) !== normalizePath(recommendedRelPath) &&
124-
normalizePath(currentModule) !== normalizePath(distRel)
125-
) {
126-
const msg = `package.json "module" property is set to "${currentModule}". It's recommended to set the "module" property to: ${recommendedRelPath}`;
162+
return;
163+
}
164+
165+
if (recommendedRelPath !== null && normalizePath(recommendedRelPath) !== normalizePath(currentModule)) {
166+
const msg = `package.json "module" property is set to "${currentModule}". It's recommended to set the "module" property to: ${normalizePath(
167+
recommendedRelPath
168+
)}`;
127169
packageJsonWarn(config, compilerCtx, buildCtx, msg, `"module"`);
128170
}
129171
};
130172

173+
/**
174+
* Get the recommended `"module"` path for `package.json` given the output
175+
* targets that a user has set on their config.
176+
*
177+
* @param config the user-supplied Stencil configuration
178+
* @returns a recommended module path or a null value to indicate no default
179+
* value is supplied
180+
*/
181+
function recommendedModulePath(config: d.Config): string | null {
182+
const customElementsBundleOT = config.outputTargets.find(isOutputTargetDistCustomElementsBundle);
183+
const customElementsOT = config.outputTargets.find(isOutputTargetDistCustomElements);
184+
const distCollectionOT = config.outputTargets.find(isOutputTargetDistCollection);
185+
186+
// If we're using `dist-custom-elements` then the preferred "module" field
187+
// value is `$OUTPUT_DIR/components/index.js`
188+
//
189+
// Additionally, the `DIST_CUSTOM_ELEMENTS` output target should override
190+
// `DIST_CUSTOM_ELEMENTS_BUNDLE` and `DIST_COLLECTION` output targets if
191+
// they're also set, so we return first with this one.
192+
if (customElementsOT) {
193+
const componentsIndexAbs = join(customElementsOT.dir, 'components', 'index.js');
194+
return relative(config.rootDir, componentsIndexAbs);
195+
}
196+
197+
if (customElementsBundleOT) {
198+
const customElementsAbs = join(customElementsBundleOT.dir, 'index.js');
199+
return relative(config.rootDir, customElementsAbs);
200+
}
201+
202+
if (distCollectionOT) {
203+
return relative(config.rootDir, join(distCollectionOT.dir, 'index.js'));
204+
}
205+
206+
// if no output target for which we define a recommended output target is set
207+
// we return `null`
208+
return null;
209+
}
210+
211+
/**
212+
* Check that the `types` field is set correctly in `package.json` for the
213+
* `DIST_COLLECTION` output target.
214+
*
215+
* @param config the stencil config
216+
* @param compilerCtx the current compiler context
217+
* @param buildCtx the current build context
218+
* @param outputTarget a DIST_COLLECTION output target
219+
*/
131220
export const validateTypes = async (
132221
config: d.Config,
133222
compilerCtx: d.CompilerCtx,
@@ -156,6 +245,15 @@ export const validateTypes = async (
156245
}
157246
};
158247

248+
/**
249+
* Check that the `collection` field is set correctly in `package.json` for the
250+
* `DIST_COLLECTION` output target.
251+
*
252+
* @param config the stencil config
253+
* @param compilerCtx the current compiler context
254+
* @param buildCtx the current build context
255+
* @param outputTarget a DIST_COLLECTION output target
256+
*/
159257
export const validateCollection = (
160258
config: d.Config,
161259
compilerCtx: d.CompilerCtx,
@@ -171,33 +269,65 @@ export const validateCollection = (
171269
}
172270
};
173271

272+
/**
273+
* Check that the `browser` field is set correctly in `package.json` for the
274+
* `DIST_COLLECTION` output target.
275+
*
276+
* @param config the stencil config
277+
* @param compilerCtx the current compiler context
278+
* @param buildCtx the current build context
279+
*/
174280
export const validateBrowser = (config: d.Config, compilerCtx: d.CompilerCtx, buildCtx: d.BuildCtx) => {
175281
if (isString(buildCtx.packageJson.browser)) {
176282
const msg = `package.json "browser" property is set to "${buildCtx.packageJson.browser}". However, for maximum compatibility with all bundlers it's recommended to not set the "browser" property and instead ensure both "module" and "main" properties are set.`;
177283
packageJsonWarn(config, compilerCtx, buildCtx, msg, `"browser"`);
178284
}
179285
};
180286

287+
/**
288+
* Build a diagnostic for an error resulting from a particular field in a
289+
* package.json file
290+
*
291+
* @param config the stencil config
292+
* @param compilerCtx the current compiler context
293+
* @param buildCtx the current build context
294+
* @param msg an error string
295+
* @param jsonField the key for the field which caused the error, used for
296+
* finding the error line in the original JSON file
297+
* @returns a diagnostic object
298+
*/
181299
const packageJsonError = (
182300
config: d.Config,
183301
compilerCtx: d.CompilerCtx,
184302
buildCtx: d.BuildCtx,
185303
msg: string,
186-
warnKey: string
187-
) => {
188-
const err = buildJsonFileError(compilerCtx, buildCtx.diagnostics, config.packageJsonFilePath, msg, warnKey);
304+
jsonField: string
305+
): d.Diagnostic => {
306+
const err = buildJsonFileError(compilerCtx, buildCtx.diagnostics, config.packageJsonFilePath, msg, jsonField);
189307
err.header = `Package Json`;
190308
return err;
191309
};
192310

311+
/**
312+
* Build a diagnostic for a warning resulting from a particular field in a
313+
* package.json file
314+
*
315+
* @param config the stencil config
316+
* @param compilerCtx the current compiler context
317+
* @param buildCtx the current build context
318+
* @param msg an error string
319+
* @param jsonField the key for the field which caused the error, used for
320+
* finding the error line in the original JSON file
321+
* @returns a diagnostic object
322+
*/
193323
const packageJsonWarn = (
194324
config: d.Config,
195325
compilerCtx: d.CompilerCtx,
196326
buildCtx: d.BuildCtx,
197327
msg: string,
198-
warnKey: string
199-
) => {
200-
const warn = buildJsonFileError(compilerCtx, buildCtx.diagnostics, config.packageJsonFilePath, msg, warnKey);
328+
jsonField: string
329+
): d.Diagnostic => {
330+
const warn = buildJsonFileError(compilerCtx, buildCtx.diagnostics, config.packageJsonFilePath, msg, jsonField);
201331
warn.header = `Package Json`;
202332
warn.level = 'warn';
203333
return warn;

‎src/utils/message-utils.ts

+17-4
Original file line numberDiff line numberDiff line change
@@ -55,33 +55,46 @@ export const buildWarn = (diagnostics: d.Diagnostic[]): d.Diagnostic => {
5555
return diagnostic;
5656
};
5757

58+
/**
59+
* Create a diagnostic message suited for representing an error in a JSON
60+
* file. This includes information about the exact lines in the JSON file which
61+
* caused the error and the path to the file.
62+
*
63+
* @param compilerCtx the current compiler context
64+
* @param diagnostics a list of diagnostics used as a return param
65+
* @param jsonFilePath the path to the JSON file where the error occurred
66+
* @param msg the error message
67+
* @param jsonField the key for the field which caused the error, used for finding
68+
* the error line in the original JSON file
69+
* @returns a reference to the newly-created diagnostic
70+
*/
5871
export const buildJsonFileError = (
5972
compilerCtx: d.CompilerCtx,
6073
diagnostics: d.Diagnostic[],
6174
jsonFilePath: string,
6275
msg: string,
63-
pkgKey: string
76+
jsonField: string
6477
) => {
6578
const err = buildError(diagnostics);
6679
err.messageText = msg;
6780
err.absFilePath = jsonFilePath;
6881

69-
if (typeof pkgKey === 'string') {
82+
if (typeof jsonField === 'string') {
7083
try {
7184
const jsonStr = compilerCtx.fs.readFileSync(jsonFilePath);
7285
const lines = jsonStr.replace(/\r/g, '\n').split('\n');
7386

7487
for (let i = 0; i < lines.length; i++) {
7588
const txtLine = lines[i];
76-
const txtIndex = txtLine.indexOf(pkgKey);
89+
const txtIndex = txtLine.indexOf(jsonField);
7790

7891
if (txtIndex > -1) {
7992
const warnLine: d.PrintLine = {
8093
lineIndex: i,
8194
lineNumber: i + 1,
8295
text: txtLine,
8396
errorCharStart: txtIndex,
84-
errorLength: pkgKey.length,
97+
errorLength: jsonField.length,
8598
};
8699
err.lineNumber = warnLine.lineNumber;
87100
err.columnNumber = txtIndex + 1;

0 commit comments

Comments
 (0)
Please sign in to comment.