Use a relative path for the declaration assets

[17cupsofcoffee]

This is intended as a fix for issue #185 - a full explanation of what the problem was can be found in this comment, but what it boiled down to is that the language service returns absolute paths for generated declaration files, and these were being used directly as keys in compilation.assets, causing Webpack to hang during the emit stage. Resolving the path relative to the current working directory beforehand fixes this issue. According to @t246246's comment on the issue, this only affects Windows, but as I don't have access to a Linux or OSX device, I can't confirm that.

One caveat to this fix is that the output file structure for the declaration files follows that of the source code - i.e, if the source looks like this:

root/
    src/
        dir1/
        dir2/

Then the declarations get output like so:

root/
    dist/
        src/
            dir1/
            dir2/
    src/
        dir1/
        dir2/

This might well be a breaking change or not be the 'correct' way of handling it - I can't tell, as this functionality never worked for me in the first place! There's a currently pending pull request on awesome-typescript-loader's repository that demonstrates a possible way of making it follow Webpack's context instead - if the current way files are output isn't acceptable, I can have a go at implementing something along those lines here instead? Let me know, I can make some more changes if needed. This should be a good start though, at the very least.

[17cupsofcoffee]

Ah, this doesn't seem to be playing nicely with the declarationOutput test (probably because the working directory is the same when running the test suite?). Might need to look into that.

[lbar]

Hi. Is there some news about this PR?

[17cupsofcoffee]

@lbar: Nothing on my end, I'm holding off on working any further on it until someone more experienced lets me know if I'm going in the right direction :p

[lbar]

OK thank you for your answer :)

[flyon]

@jbrantly @johnnyreilly or any other maintainer, can you please look at this simple PR fixing #185, people are leaving to awesome-typescript-loader because of this issue, and I'd hate to join them!

[johnnyreilly]

Okay - running the test just hangs on windows. Manually testing the PR does change the emit path but it also allows the test to run; this is a better outcome that at present in my book.

I need to ponder this a bit.

[johnnyreilly]

Okay - I've pondered it; I can't see any reason not to merge. @jbrantly if there's any objections then please do pitch in; it's a 1 line change and can easily be amended if there's abetter approach possible. But since this is not a working thing (at least on Windows) then I think it's probably worth going with this for now.

Just as a by the by I put some console.logs in to test various things out:

                    console.log("declarationFile", JSON.stringify(declarationFile))
                    console.log("process.cwd()", process.cwd())
                    let assetPath = path.relative(process.cwd(), declarationFile.name);
                    let assetPath2 = path.normalize(declarationFile.name);
                    console.log("assetPath2", assetPath2)
                    console.log("assetPath", assetPath)

  declarationOutput
declarationFile {"name":"C:/source/ts-loader/.test/declarationOutput/app.d.ts","text":"import dep = require('./sub/dep')
;\ndeclare class Test extends dep {\n    doSomething(): void;\n}\nexport = Test;\n"}
process.cwd() C:\source\ts-loader
assetPath2 C:\source\ts-loader\.test\declarationOutput\app.d.ts
assetPath .test\declarationOutput\app.d.ts

declarationFile {"name":"C:/source/ts-loader/.test/declarationOutput/sub/dep.d.ts","text":"declare class Test {\n    doSomething(): void;\n}\nexport = Test;\n"}
process.cwd() C:\source\ts-loader
assetPath2 C:\source\ts-loader\.test\declarationOutput\sub\dep.d.ts
assetPath .test\declarationOutput\sub\dep.d.ts

Do notice the difference in paths "/" vs "". Just didn't want to lose this information.

[17cupsofcoffee]

@johnnyreilly Just as an aside, I've just ticked "Allow edits from maintainers" on this PR, so if you do need to make any changes you should have write access to my branch now!

[johnnyreilly]

Okay merged. Now to try and get us back to a "passing tests" state 😄

[dizel3d]

This fix also gave us a broken change. d.ts files paths should be related to webpack context, not to current process workdir. They are not always equal. My webpack config looks like:

module.exports = {
    context: path.join(__dirname, 'src')
    ...
}

In 0.8.1 my asset name was index.d.ts, but now in 0.9.0 it is src\index.d.ts.

[johnnyreilly]

Hi @dizel3d,

Thanks for reporting; PRs to improve the behaviour would be gratefully received. Over to you 👍

[dizel3d]

@johnnyreilly, I did it, see PR #307

[17cupsofcoffee]

@dizel3d - Apologies for the breaking PR, this was my first one outside of updating docs and I wasn't too familiar with what the expected behavior was! Thanks for improving it <3

[johnnyreilly]

@17cupsofcoffee - don't let it frighten you off submitting more PRs!