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.

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

Fix/collection output transpile imports #3523

Merged
merged 11 commits into from
Sep 8, 2022
Merged
3 changes: 2 additions & 1 deletion src/compiler/config/outputs/validate-collection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ export const validateCollection = (
return userOutputs.filter(isOutputTargetDistCollection).map((outputTarget) => {
return {
...outputTarget,
dir: getAbsolutePath(config, outputTarget.dir || 'dist/collection'),
transformAliasedImportPaths: outputTarget.transformAliasedImportPaths ?? false,
dir: getAbsolutePath(config, outputTarget.dir ?? 'dist/collection'),
};
});
};
2 changes: 2 additions & 0 deletions src/compiler/config/outputs/validate-dist.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ export const validateDist = (config: d.ValidatedConfig, userOutputs: d.OutputTar
dir: distOutputTarget.dir,
collectionDir: distOutputTarget.collectionDir,
empty: distOutputTarget.empty,
transformAliasedImportPaths: distOutputTarget.transformAliasedImportPathsInCollection,
});
outputs.push({
type: COPY,
Expand Down Expand Up @@ -134,6 +135,7 @@ const validateOutputTargetDist = (config: d.ValidatedConfig, o: d.OutputTargetDi
copy: validateCopy(o.copy ?? [], []),
polyfills: isBoolean(o.polyfills) ? o.polyfills : undefined,
empty: isBoolean(o.empty) ? o.empty : true,
transformAliasedImportPathsInCollection: o.transformAliasedImportPathsInCollection ?? false,
};

if (!isAbsolute(outputTarget.buildDir)) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
import type * as d from '@stencil/core/declarations';
import { validateConfig } from '../validate-config';
import { mockConfig, mockLoadConfigInit } from '@stencil/core/testing';
import { resolve, join } from 'path';

describe('validateDistCollectionOutputTarget', () => {
let config: d.Config;

const rootDir = resolve('/');
const defaultDir = join(rootDir, 'dist', 'collection');

beforeEach(() => {
config = mockConfig();
});

it('sets correct default values', () => {
const target: d.OutputTargetDistCollection = {
type: 'dist-collection',
empty: false,
dir: null,
collectionDir: null,
};
config.outputTargets = [target];

const { config: validatedConfig } = validateConfig(config, mockLoadConfigInit());

expect(validatedConfig.outputTargets).toEqual([
{
type: 'dist-collection',
empty: false,
dir: defaultDir,
collectionDir: null,
transformAliasedImportPaths: false,
},
]);
});

it('sets specified directory', () => {
const target: d.OutputTargetDistCollection = {
type: 'dist-collection',
empty: false,
dir: '/my-dist',
collectionDir: null,
};
config.outputTargets = [target];

const { config: validatedConfig } = validateConfig(config, mockLoadConfigInit());

expect(validatedConfig.outputTargets).toEqual([
{
type: 'dist-collection',
empty: false,
dir: '/my-dist',
collectionDir: null,
transformAliasedImportPaths: false,
},
]);
});

describe('transformAliasedImportPaths', () => {
it.each([false, true])(
"sets option '%s' when explicitly '%s' in config",
(transformAliasedImportPaths: boolean) => {
const target: d.OutputTargetDistCollection = {
type: 'dist-collection',
empty: false,
dir: null,
collectionDir: null,
transformAliasedImportPaths,
};
config.outputTargets = [target];

const { config: validatedConfig } = validateConfig(config, mockLoadConfigInit());

expect(validatedConfig.outputTargets).toEqual([
{
type: 'dist-collection',
empty: false,
dir: defaultDir,
collectionDir: null,
transformAliasedImportPaths,
},
]);
}
);
});
});
88 changes: 88 additions & 0 deletions src/compiler/config/test/validate-output-dist.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ describe('validateDistOutputTarget', () => {
type: 'dist',
polyfills: undefined,
typesDir: path.join(rootDir, 'my-dist', 'types'),
transformAliasedImportPathsInCollection: false,
},
{
esmDir: path.join(rootDir, 'my-dist', 'my-build', 'testing'),
Expand Down Expand Up @@ -62,6 +63,7 @@ describe('validateDistOutputTarget', () => {
collectionDir: path.join(rootDir, 'my-dist', 'collection'),
dir: path.join(rootDir, '/my-dist'),
empty: false,
transformAliasedImportPaths: false,
type: 'dist-collection',
},
{
Expand Down Expand Up @@ -107,4 +109,90 @@ describe('validateDistOutputTarget', () => {
const { config } = validateConfig(userConfig, mockLoadConfigInit());
expect(config.outputTargets.some((o) => o.type === 'dist')).toBe(false);
});

it('sets option to transform aliased import paths when enabled', () => {
const outputTarget: d.OutputTargetDist = {
type: 'dist',
dir: 'my-dist',
buildDir: 'my-build',
empty: false,
transformAliasedImportPathsInCollection: true,
};
userConfig.outputTargets = [outputTarget];
userConfig.buildDist = true;

const { config } = validateConfig(userConfig, mockLoadConfigInit());

expect(config.outputTargets).toEqual([
{
buildDir: path.join(rootDir, 'my-dist', 'my-build'),
collectionDir: path.join(rootDir, 'my-dist', 'collection'),
copy: [],
dir: path.join(rootDir, 'my-dist'),
empty: false,
esmLoaderPath: path.join(rootDir, 'my-dist', 'loader'),
type: 'dist',
polyfills: undefined,
typesDir: path.join(rootDir, 'my-dist', 'types'),
transformAliasedImportPathsInCollection: true,
},
{
esmDir: path.join(rootDir, 'my-dist', 'my-build', 'testing'),
empty: false,
isBrowserBuild: true,
legacyLoaderFile: path.join(rootDir, 'my-dist', 'my-build', 'testing.js'),
polyfills: true,
systemDir: undefined,
systemLoaderFile: undefined,
type: 'dist-lazy',
},
{
copyAssets: 'dist',
copy: [],
dir: path.join(rootDir, 'my-dist', 'my-build', 'testing'),
type: 'copy',
},
{
file: path.join(rootDir, 'my-dist', 'my-build', 'testing', 'testing.css'),
type: 'dist-global-styles',
},
{
dir: path.join(rootDir, 'my-dist'),
type: 'dist-types',
typesDir: path.join(rootDir, 'my-dist', 'types'),
},
{
collectionDir: path.join(rootDir, 'my-dist', 'collection'),
dir: path.join(rootDir, '/my-dist'),
empty: false,
transformAliasedImportPaths: true,
type: 'dist-collection',
},
{
copy: [{ src: '**/*.svg' }, { src: '**/*.js' }],
copyAssets: 'collection',
dir: path.join(rootDir, 'my-dist', 'collection'),
type: 'copy',
},
{
type: 'dist-lazy',
cjsDir: path.join(rootDir, 'my-dist', 'cjs'),
cjsIndexFile: path.join(rootDir, 'my-dist', 'index.cjs.js'),
empty: false,
esmDir: path.join(rootDir, 'my-dist', 'esm'),
esmEs5Dir: undefined,
esmIndexFile: path.join(rootDir, 'my-dist', 'index.js'),
polyfills: true,
},
{
cjsDir: path.join(rootDir, 'my-dist', 'cjs'),
componentDts: path.join(rootDir, 'my-dist', 'types', 'components.d.ts'),
dir: path.join(rootDir, 'my-dist', 'loader'),
empty: false,
esmDir: path.join(rootDir, 'my-dist', 'esm'),
esmEs5Dir: undefined,
type: 'dist-lazy-loader',
},
]);
});
});
39 changes: 34 additions & 5 deletions src/compiler/output-targets/dist-collection/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,20 @@ import { catchError, COLLECTION_MANIFEST_FILE_NAME, flatOne, generatePreamble, n
import { isOutputTargetDistCollection } from '../output-utils';
import { join, relative } from 'path';
import { typescriptVersion, version } from '../../../version';
import ts from 'typescript';
import { mapImportsToPathAliases } from '../../transformers/map-imports-to-path-aliases';

/**
* Main output target function for `dist-collection`. This function takes the compiled output from a
* {@link ts.Program}, runs each file through a transformer to transpile import path aliases, and then writes
* the output code and source maps to disk in the specified collection directory.
*
* @param config The validated Stencil config.
* @param compilerCtx The current compiler context.
* @param buildCtx The current build context.
* @param changedModuleFiles The changed modules returned from the TS compiler.
* @returns An empty promise. Resolved once all functions finish.
*/
export const outputCollection = async (
tanner-reits marked this conversation as resolved.
Show resolved Hide resolved
config: d.ValidatedConfig,
compilerCtx: d.CompilerCtx,
Expand All @@ -27,15 +40,31 @@ export const outputCollection = async (
const mapCode = mod.sourceMapFileText;

await Promise.all(
outputTargets.map(async (o) => {
outputTargets.map(async (target) => {
const relPath = relative(config.srcDir, mod.jsFilePath);
const filePath = join(o.collectionDir, relPath);
await compilerCtx.fs.writeFile(filePath, code, { outputTargetType: o.type });
const filePath = join(target.collectionDir, relPath);

// Transpile the already transpiled modules to apply
// a transformer to convert aliased import paths to relative paths
// We run this even if the transformer will perform no action
// to avoid race conditions between multiple output targets that
// may be writing to the same location
const { outputText } = ts.transpileModule(code, {
Copy link
Member Author

Choose a reason for hiding this comment

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

Only downside to this approach is, for this target, we'll essentially end up transpiling each module twice. However, I ran a couple builds with this change and without and only seemed to affect Stencil build times within a project by a second or so (that testing was done with the framework code base)

Copy link
Member

Choose a reason for hiding this comment

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

I was able to see the same thing:

27318d75df60dfce1a90f23ba31ea2b6636ba42f of Ionic Framework:

[30:11.6]  @stencil/core
[30:11.7]  v2.17.4 🐞
[30:13.6]  build, ionic, prod mode, started ...
[30:13.6]  transpile started ...
[30:17.5]  transpile finished in 3.95 s
[30:17.5]  copy started ...
[30:17.5]  generate custom elements started ...
[30:17.5]  generate hydrate app started ...
[30:17.5]  generate lazy started ...
[30:20.4]  copy finished (1348 files) in 2.87 s
[30:31.6]  generate custom elements finished in 14.11 s
[30:32.5]  generate hydrate app finished in 14.92 s
[30:37.9]  generate lazy finished in 20.32 s
[30:37.9]  generating react-library started ...
[30:37.9]  generating vue-library started ...
[30:37.9]  generating angular-library started ...
[30:37.9]  generate vue-library finished in 9 ms
[30:37.9]  generate angular-library finished in 9 ms
[30:37.9]  generate react-library finished in 11 ms
[30:38.0]  build finished in 24.44 s

> @ionic/core@6.2.3 cdnloader
> node scripts/copy-cdn-loader.js

npm run build  82.05s user 4.00s system 294% cpu 29.173 total

This branch:

[31:04.5]  @stencil/core
[31:04.6]  [LOCAL DEV] 💡
[31:06.5]  build, ionic, prod mode, started ...
[31:06.5]  transpile started ...
[31:10.5]  transpile finished in 4.05 s
[31:10.5]  copy started ...
[31:10.5]  generate custom elements started ...
[31:10.5]  generate hydrate app started ...
[31:10.5]  generate lazy started ...
[31:14.3]  copy finished (1348 files) in 3.73 s
[31:23.0]  generate custom elements finished in 12.47 s
[31:24.8]  generate hydrate app finished in 14.23 s
[31:30.0]  generate lazy finished in 19.44 s
[31:30.0]  generating react-library started ...
[31:30.0]  generating vue-library started ...
[31:30.0]  generating angular-library started ...
[31:30.0]  generate angular-library finished in 9 ms
[31:30.0]  generate vue-library finished in 9 ms
[31:30.0]  generate react-library finished in 10 ms
[31:30.1]  build finished in 23.66 s


> @ionic/core@6.2.3 cdnloader
> node scripts/copy-cdn-loader.js

npm run build  81.57s user 3.79s system 308% cpu 27.655 total

fileName: mod.sourceFilePath,
compilerOptions: {
target: ts.ScriptTarget.Latest,
},
transformers: {
after: [mapImportsToPathAliases(config, filePath, target)],
},
});

await compilerCtx.fs.writeFile(filePath, outputText, { outputTargetType: target.type });

if (mod.sourceMapPath) {
const relativeSourceMapPath = relative(config.srcDir, mod.sourceMapPath);
const sourceMapOutputFilePath = join(o.collectionDir, relativeSourceMapPath);
await compilerCtx.fs.writeFile(sourceMapOutputFilePath, mapCode, { outputTargetType: o.type });
const sourceMapOutputFilePath = join(target.collectionDir, relativeSourceMapPath);
await compilerCtx.fs.writeFile(sourceMapOutputFilePath, mapCode, { outputTargetType: target.type });
}
})
);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import { outputCollection } from '../dist-collection';
import type * as d from '../../../declarations';
import { mockValidatedConfig, mockBuildCtx, mockCompilerCtx, mockModule } from '@stencil/core/testing';
import * as test from '../../transformers/map-imports-to-path-aliases';
import { normalize } from 'path';

describe('Dist Collection output target', () => {
let mockConfig: d.ValidatedConfig;
let mockedBuildCtx: d.BuildCtx;
let mockedCompilerCtx: d.CompilerCtx;
let changedModules: d.Module[];

let mapImportPathSpy: jest.SpyInstance;

const mockTraverse = jest.fn().mockImplementation((source: any) => source);
const mockMap = jest.fn().mockImplementation(() => mockTraverse);
const target: d.OutputTargetDistCollection = {
type: 'dist-collection',
dir: '',
collectionDir: '/dist/collection',
};

beforeEach(() => {
mockConfig = mockValidatedConfig({
srcDir: '/src',
});
mockedBuildCtx = mockBuildCtx();
mockedCompilerCtx = mockCompilerCtx();
changedModules = [
mockModule({
staticSourceFileText: '',
jsFilePath: '/src/main.js',
sourceFilePath: '/src/main.ts',
}),
];

jest.spyOn(mockedCompilerCtx.fs, 'writeFile');

mapImportPathSpy = jest.spyOn(test, 'mapImportsToPathAliases');
mapImportPathSpy.mockReturnValue(mockMap);
});

afterEach(() => {
jest.restoreAllMocks();
});

describe('transform aliased import paths', () => {
// These tests ensure that the transformer for import paths is called regardless
// of the config value (the function will decided whether or not to actually do anything) to avoid
// a race condition with duplicate file writes
it.each([true, false])(
'calls function to transform aliased import paths when the output target config flag is `%s`',
async (transformAliasedImportPaths: boolean) => {
mockConfig.outputTargets = [
{
...target,
transformAliasedImportPaths,
},
];

await outputCollection(mockConfig, mockedCompilerCtx, mockedBuildCtx, changedModules);

expect(mapImportPathSpy).toHaveBeenCalledWith(mockConfig, normalize('/dist/collection/main.js'), {
collectionDir: '/dist/collection',
dir: '',
transformAliasedImportPaths,
type: 'dist-collection',
});
expect(mapImportPathSpy).toHaveBeenCalledTimes(1);
}
);
});
});
Loading