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

Refactored to use canonical file names #171

Merged
merged 5 commits into from
Aug 5, 2016
Merged

Conversation

cartant
Copy link
Contributor

@cartant cartant commented Aug 3, 2016

@basarat What are your thoughts on this change? It's a fix for #161 - which, it turns out, is a tsify problem and not a TypeScript problem.

It seems that tsify's implementation of getCanonicalFileName has always returned a relative path. That behaviour differs significantly from TypeScript's implementation - which returns either the original path or a lower-case copy of the path. With the changes made in TypeScript 2.0, tsify's implementation now breaks the getSourceFilePathInNewDir function (in some circumstances).

I have searched through the TypeScript source and it seems to me that getCanonicalFileName is used only when comparing paths, so its output shouldn't affect paths seen by tsify.

Can you think of any reason why the tsify implementation would be returning a relative path?

This is a bug that's preventing me from using TypeScript 2.0, so I'd like to get it fixed. With the change, my TypeScript 1.8 builds are fine. (Although, I only use lower-case file names - so some further investigation will be required.) However, it's getting late, here, and I've made enough mistakes for the day, so I'll wait until tomorrow to install TypeScript 2.0 to see how it behaves.

@cartant
Copy link
Contributor Author

cartant commented Aug 4, 2016

@basarat I've taken this PR further and I've refactored the Tsifier and the Host to remove the conversion of pretty much everything to relative paths. I have everything working and I think it's now simpler and easier to understand. However, I have a question regarding this:

if (normalized === '__lib.d.ts')
    return this.libDefault;

I'm not seeing a __lib.d.ts file show up in my build. Does it have something to do with an older version of TypeScript? If so, should I be expecting it to be passed just as a file name or would it have a path? The current implementation performs the above test on the relative path that's the output of _normalizedRelative and it's not clear what I should be looking for if the relative path conversion is not performed.

@basarat
Copy link
Member

basarat commented Aug 4, 2016

I'm not seeing a __lib.d.ts file show up in my build

I've never seen it either. It makes no sense. Must be a very old (before 0.8.1) build :)

@cartant
Copy link
Contributor Author

cartant commented Aug 4, 2016

Thanks. I have the refactoring done and all of the tests pass. I'll run all of my builds and if they're all okay, I'll update this PR so that you can have a look at it.

@cartant cartant changed the title TypeScript implementation of getCanonicalFileName Refactored to use canonical file names Aug 4, 2016
@cartant
Copy link
Contributor Author

cartant commented Aug 4, 2016

@basarat Could you have a look over this PR? Changes can be summarized as follows:

  • I've refactored the in-memory cache to use canonical paths, rather than normalized, relative paths. (This should fix Referring to the same file with different casing causes empty output #122.)
  • The Tsifier no longer reaches into the Host's internals (to determine the root file names and to lookup output).
  • Pretty much all path manipulation (apart from the source map paths) and transformation is now in the Host.

@basarat
Copy link
Member

basarat commented Aug 4, 2016

Difficult to know without actually using. I trust you and you 👐 completely 🙌 🌹

@cartant
Copy link
Contributor Author

cartant commented Aug 4, 2016

If there's nothing that stands out as being an obvious error and if I haven't just made things more confusing, that's good.

It seems that I have some further TypeScript 2.0 issues to get sorted. I'll continue to use this branch in anger for a while before I think about merging it.

@cartant
Copy link
Contributor Author

cartant commented Aug 4, 2016

@basarat My TypeScript 2.0 problems seem to be related to the Host's writeFile not being called for some files imported from node_modules. This seems to be specific to 2.0.0, as it works in 1.8.10 and in 2.1.0-dev.20160804, so I don't think it's a tsify thing.

One thing I have noticed though is that 2.1.0 seems to be behaving quite differently. When tsify does its initial compile, it passes in the tsconfig.json files (the index.d.ts from typings, in my case) and the Browserify entry point(s). This used to be the only compilation required, as the Host's writeFile would be called for any susequently imported files - as the compilation was performed. However, with 2.1.0, writeFile seems to be called only for the files passed to the initial compile. This means that there need to be subsequent re-compilations with each cache miss. In my small test harness, this results in three compilations instead of one. In typical project, there will be many more. Has the compile interface changed in any way? Will tsify need to do something different with more recent versions of TypeScript?

@cartant
Copy link
Contributor Author

cartant commented Aug 5, 2016

@basarat Regarding typescript@next, it seems to be the behaviour of imports under node_modules that has changed. It's only when there's a cache miss - and a node_modules path is explicitly added to the files - that a file under node_modules is written to the host. Is there some setting or option that controls this, as anything under node_modules needs to be bundled and tsify is currently recompiling upon each cache miss.

@basarat
Copy link
Member

basarat commented Aug 5, 2016

and a node_modules path is explicitly added to the files

TypeScript files get picked up with moduleResolution node. I'm probably unclear on your question I think 🌹

@cartant
Copy link
Contributor Author

cartant commented Aug 5, 2016

Yeah, moduleResolution is set to node. The change in behaviour seems to be that typescript@next doesn't emit (that is, call the Host's writeFile function) on files under node_modules unless they are in the root files passed to createProgram - which is what happens after a cache miss.

@cartant
Copy link
Contributor Author

cartant commented Aug 5, 2016

The repeated compilations make tsify pretty inefficient. Which is a bummer, as I've just added the following of symlinks to fix #135 and #150.

@cartant
Copy link
Contributor Author

cartant commented Aug 5, 2016

Hmm. If I run tsc, it does write the imported node_modules files to the outDir, so maybe there's something more subtle going on - as the files under node_modules that are emitted by tsc should really have corresponding calls to the Host's writeFile in tsify.

@cartant
Copy link
Contributor Author

cartant commented Aug 5, 2016

@basarat Nope, I was wrong about tsc - I was still running a global version of 1.8.10.

If I run tsc using typescript@next no files under node_modules are written to the outDir. Are you aware of any option or setting that could be used to make tsc revert to the previous behaviour and emit files under node_modules?

@cartant
Copy link
Contributor Author

cartant commented Aug 5, 2016

@basarat Reading the following PR/issue discussions, it seems that the change in behaviour (re: node_modules) is by-design (unfortunately):

@basarat
Copy link
Member

basarat commented Aug 5, 2016

is by-design

Yup 🌹

@cartant
Copy link
Contributor Author

cartant commented Aug 5, 2016

It's not the end of the world. tsify gets to see the Host parsing the files under node_modules, so the worst-case scenario is one additional compile.

@cartant
Copy link
Contributor Author

cartant commented Aug 5, 2016

I'll give this branch a bit more of a workout with my stuff and then I think I'll merge it. I believe it will fix the three outstanding bugs.

I'll put the typescript@next-compatibility stuff into a new branch. There is still something weird happening with 2.0, but I think it's a bug in TypeScript. I've not spent too much time looking into it.

@cartant cartant removed the discussion label Aug 5, 2016
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