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

Completely ignore tsconfig.json if project === false. #523

Closed
wants to merge 1 commit into from

Conversation

NaridaL
Copy link
Contributor

@NaridaL NaridaL commented Jan 22, 2018

Fixes #456
process env variables are always strings, see https://nodejs.org/api/process.html#process_process_env,
so I added TS_NODE_NO_PROJECT to set project === false
Added corresponding test.

@NaridaL
Copy link
Contributor Author

NaridaL commented Jan 22, 2018

Simpler/more obvious behavior would be to just read tsconfig if project is explicitly passed. But that would be a breaking change, hence this somewhat roundabout change.

Fixes TypeStrong#456
process env variables are always strings, see https://nodejs.org/api/process.html#process_process_env,
so I added TS_NODE_NO_PROJECT to set project === false
@coveralls
Copy link

coveralls commented Jan 22, 2018

Coverage Status

Coverage increased (+0.3%) to 77.698% when pulling 229d37a on NaridaL:master into d5a941c on TypeStrong:master.

@NaridaL
Copy link
Contributor Author

NaridaL commented Feb 14, 2018

@blakeembrey Any comment? Do you need help maintaining this repository?

@gutenye
Copy link

gutenye commented Feb 18, 2018

When will this one get merged?

@blakeembrey
Copy link
Member

This change wouldn't work, you're returning an empty configuration object without merging any provided configuration options and without merging the overriding which are required for ts-node to work properly.

@NaridaL
Copy link
Contributor Author

NaridaL commented Feb 19, 2018

@blakeembrey I don't follow. The behavior of loadSync is as follows:

function loadSync(cwd, filename) {
    var path = resolveSync(cwd, filename);
    if (path == null) {
        return {
            config: {
                files: [],
                compilerOptions: {}
            }
        };
    }
    var config = readFileSync(path);
    return { path: path, config: config };
}

I.e. it returns { config: { files: [], compilerOptions: {} } } if there is no tsconfig/it can't find the tsconfig. readConfig now returns the same value it would if it cannot find the tsconfig when project === false and the same value as it previously did in all other cases.

@blakeembrey
Copy link
Member

What about all of

ts-node/src/index.ts

Lines 457 to 476 in d5a941c

// Override default configuration options.
result.config.compilerOptions = Object.assign({}, result.config.compilerOptions, compilerOptions, {
sourceMap: true,
inlineSourceMap: false,
inlineSources: true,
declaration: false,
noEmit: false,
outDir: '$$ts-node$$'
})
const configPath = result.path && normalizeSlashes(result.path)
const basePath = configPath ? dirname(configPath) : normalizeSlashes(cwd)
if (typeof ts.parseConfigFile === 'function') {
return fixConfig(ts.parseConfigFile(result.config, ts.sys, basePath), ts)
}
if (typeof ts.parseJsonConfigFileContent === 'function') {
return fixConfig(ts.parseJsonConfigFileContent(result.config, ts.sys, basePath, undefined, configPath), ts)
}
though?

@@ -95,7 +95,7 @@ const DEFAULTS = {
cacheDirectory: process.env['TS_NODE_CACHE_DIRECTORY'],
compiler: process.env['TS_NODE_COMPILER'],
compilerOptions: parse(process.env['TS_NODE_COMPILER_OPTIONS']),
project: process.env['TS_NODE_PROJECT'],
project: process.env['TS_NODE_NO_PROJECT'] ? false : process.env['TS_NODE_PROJECT'],
Copy link
Member

Choose a reason for hiding this comment

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

Use yn like the rest of the options.

@blakeembrey
Copy link
Member

I'm rewriting this module now too, so don't worry too much about this - I'm trying to simplify much of the things that are broken today.

@blakeembrey
Copy link
Member

Closing with #536, but feel free to reopen if it's still an issue after release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants