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.

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

Jump to bottom

support TypeScript-format config #1253

Merged
merged 9 commits into from
Oct 18, 2020
Merged

support TypeScript-format config #1253

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
33c2f62
support TypeScript-format config
g-plane Oct 8, 2020
7e7fa8b
update docs
g-plane Oct 8, 2020
d42f274
fix lint error
g-plane Oct 8, 2020
6a15653
re-implement importing TS-format config
g-plane Oct 9, 2020
0b7e5a5
add compat usage
g-plane Oct 9, 2020
f90b0dd
build TS-format config file with "esbuild"
g-plane Oct 9, 2020
61156f2
add error message when failed to load config
g-plane Oct 17, 2020
49acf4e
update snapshot
g-plane Oct 17, 2020
d3d99ac
Update 10-reference.md
FredKSchott Oct 18, 2020
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
27 changes: 26 additions & 1 deletion snowpack/src/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@ import buildScriptPlugin from '@snowpack/plugin-build-script';
import runScriptPlugin from '@snowpack/plugin-run-script';
import {cosmiconfigSync} from 'cosmiconfig';
import {all as merge} from 'deepmerge';
import esbuild from 'esbuild';
import http from 'http';
import {validate, ValidatorResult} from 'jsonschema';
import os from 'os';
import path from 'path';
import yargs from 'yargs-parser';

Expand Down Expand Up @@ -826,13 +828,36 @@ export function createConfiguration(

export function loadAndValidateConfig(flags: CLIFlags, pkgManifest: any): SnowpackConfig {
const explorerSync = cosmiconfigSync(CONFIG_NAME, {
// only support these 4 types of config for now
// only support these 5 types of config for now
searchPlaces: [
'package.json',
'snowpack.config.cjs',
'snowpack.config.js',
'snowpack.config.ts',
'snowpack.config.json',
],
loaders: {
Copy link
Owner

Choose a reason for hiding this comment

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

strong +1 on this feature request, but I think this implementation will break any relative imports (../ out of the temp dir is going to point somewhere else).

I'm not sure of a better way to do this though. We could create the '.snowpack.config.cjs' file alongside snowpack.config.ts temporarily, and then delete it when we're done. It sounds like cosmicconfig does some caching, which means this wouldn't have to happen every time. That's the best I can come up with right now

Copy link
Contributor Author

@g-plane g-plane Oct 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Outdated. Please read next comment.

I've re-implemented this part (see latest commit). The new way is using Node.js built-in vm module and execute it. Because the code is wrapped in a function, variables created in the config file won't be leaked into outer scope. And, there won't be filesystem operations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now the config file will be bundled by esbuild. That means any imports will be bundled into a single file, so placing the compiled file in temp directory should be safe now.

Copy link
Owner

Choose a reason for hiding this comment

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

That's clever! It's definitely a solution to the relative imports problem but introduces a lot of extra effort with potential for slow down and bugs. I wouldn't feel comfortable unless this also came with a console.warn("TypeScript config support is still experimental").

tbh I still prefer the temporary single-file method outlined in https://github.com/pikapkg/snowpack/pull/1253/files#r501834625 to bundling the entire config. A light touch for a workaround like this is valuable. And, since cosmicconfig does caching, the users should rarely notice it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While we can put the temporary file into current working directory, I think bundling is necessary, because this allows to import other TS files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ping @FredKSchott

Copy link
Owner

Choose a reason for hiding this comment

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

still thinking about this. St the very least, we want to get the current release that's on master out first, and then revisit this post-release. Hopefully getting that out tonight/tomorrow.

'.ts': (configPath) => {
const outPath = path.join(os.tmpdir(), '.snowpack.config.cjs');

try {
esbuild.buildSync({
entryPoints: [configPath],
outfile: outPath,
bundle: true,
platform: 'node',
});

const exported = require(outPath);
return exported.default || exported;
} catch (error) {
logger.error(
'Warning: TypeScript config file support is still experimental. Consider moving to JavaScript if you continue to have problems.',
);
throw error;
}
},
},
// don't support crawling up the folder tree:
stopDir: path.dirname(process.cwd()),
});
Expand Down
12 changes: 12 additions & 0 deletions test/build/config-ts-format/__snapshots__
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`snowpack build config-ts-format: allFiles 1`] = `
Array [
"__snowpack__/env.js",
"_dist_/index.js",
]
`;

exports[`snowpack build config-ts-format: build/__snowpack__/env.js 1`] = `"export default {\\"MODE\\":\\"production\\",\\"NODE_ENV\\":\\"production\\"};"`;

exports[`snowpack build config-ts-format: build/_dist_/index.js 1`] = `"console.log('fooey');"`;
1 change: 1 addition & 0 deletions test/build/config-ts-format/export.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default '/_dist_';
12 changes: 12 additions & 0 deletions test/build/config-ts-format/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{
"private": true,
"version": "1.0.1",
"name": "@snowpack/test-config-ts-format",
"description": "Test for TypeScript-format config file.",
"scripts": {
"testbuild": "snowpack build"
},
"devDependencies": {
"snowpack": "^2.12.1"
}
}
10 changes: 10 additions & 0 deletions test/build/config-ts-format/snowpack.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import type {SnowpackConfig} from '../../../snowpack/src';
import dist from './export';

const config: Partial<SnowpackConfig> = {
mount: {
'./src': dist,
},
};

export default config;
1 change: 1 addition & 0 deletions test/build/config-ts-format/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
console.log('fooey');