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

node modules subdir #365

Closed
wants to merge 4 commits into from
Closed

node modules subdir #365

wants to merge 4 commits into from

Conversation

basarat
Copy link
Member

@basarat basarat commented Nov 9, 2016

refs #278

Run as npm run comparison-tests -- --single-test node_modules-subdir

Or cd into test dir, edit webpack.config.js in the test dir to have an additional .. and run as webpack in the test dir (with npm install webpack -g)

[0] ./.test/node_modules-subdir/app.ts 62 bytes {0} [built] [1 error]

ERROR in ./.test/node_modules-subdir/~/foo/bar/index.ts
Copy link
Member Author

Choose a reason for hiding this comment

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

This should not error

@basarat
Copy link
Member Author

basarat commented Nov 9, 2016

Traced the bug to TypeScript. Modified the language service getEmitOutput as follows:

 function getEmitOutput(fileName, emitDeclarationsOnly) {
            synchronizeHostData();
            var sourceFile = getValidSourceFile(fileName);
            var outputFiles = [];
            function writeFile(fileName, data, writeByteOrderMark) {
                outputFiles.push({
                    name: fileName,
                    writeByteOrderMark: writeByteOrderMark,
                    text: data
                });
            }

            var emitOutput = program.emit(sourceFile, writeFile, cancellationToken, emitDeclarationsOnly);

            if (fileName.lastIndexOf('lib') == -1) {
              console.log('getEmitOutput'.magenta, fileName, outputFiles.length)
              console.log(sourceFile.getFullText())
            }

            return {
                outputFiles: outputFiles,
                emitSkipped: emitOutput.emitSkipped
            };
        }

And in the log I can see that it outputs nothing even though it sees some input text:

getEmitOutput /Users/syedb/REPOS/tmp/ts-loader/test/comparison-tests/node_modules-subdir/node_modules/foo/bar/index.ts 0
export const bar = "sample"

image

Will investigate more later

@basarat
Copy link
Member Author

basarat commented Nov 9, 2016

Got it. Its the function function forEachExpectedEmitFile(host, action, targetSourceFile, emitOnlyDtsFiles) { in typescript.js

The function body is super simple:

function forEachExpectedEmitFile(host, action, targetSourceFile, emitOnlyDtsFiles) {

      console.log(targetSourceFile.fileName.red, {external:host.isSourceFileFromExternalLibrary(targetSourceFile)}); // My addition

        var options = host.getCompilerOptions();
        // Emit on each source file
        if (options.outFile || options.out) {
            onBundledEmit(host);
        }
        else {
            var sourceFiles = targetSourceFile === undefined ? host.getSourceFiles() : [targetSourceFile];
            for (var _i = 0, sourceFiles_1 = sourceFiles; _i < sourceFiles_1.length; _i++) {
                var sourceFile = sourceFiles_1[_i];
                // Don't emit if source file is a declaration file, or was located under node_modules
                if (!isDeclarationFile(sourceFile) && !host.isSourceFileFromExternalLibrary(sourceFile)) {
                    onSingleFileEmit(host, sourceFile);
                }
            }
        }

Basically onSingleFileEmit is not called on this file as its considered an externalLibrary This is also shown below:

image

Seems intentional in the TypeScript's design to not allow external libraries to ship .ts files and instead provide .d.ts + .js file. I'll punt on this for now and get some sleep before raising to the TS team 🌹

@basarat
Copy link
Member Author

basarat commented Nov 9, 2016

Thinking this through, for our (ts-loader) host, isSourceFileFromExternalLibrary should always be false. If a ts file has made its way into require it needs to be emitted. I'll write the code + document it accordingly the next time I'm in an OSS mode :) ❤️

@johnnyreilly
Copy link
Member

Wow! I go to do nursery drop off and check my phone to find this. Good work @basarat :+100: 🌷

@basarat
Copy link
Member Author

basarat commented Nov 14, 2016

Closing since I think its a bad idea to have the need to transpile .ts files from node_modules. Continuing discussion here : #278 🌹

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.

2 participants