Add broccoli-typescript-compiler test

[weswigham]

Fixes #32529

Their build is currently blocked on an out of date @types/node (which afaik is a known 3.6 break), so there's a RUN yarn add @types/node@10 --ignore-scripts in the test to forcibly upgrade it - we should remove that once it's been updated in the project. We build the project just fine, but their tests fail - looks like we've got a hard requirement on realpath existing on some host now?

[DanielRosenwasser]

looks like we've got a hard requirement on realpath existing on some host now?

Yeah, @wycats raised tildeio/broccoli-typescript-compiler#79 to me, and it's part of why I opened up the original issue. Any idea if this was intentional? (@sheetalkamat?)

[sheetalkamat]

matchFiles is internal and not exposed outside. #30376 fixes the issue with recusive symbol links which needed change to this method. I am not sure we can maintain signatures for functions that are not exposed as API ?

[weswigham]

It looks like parseJsonSourceFileConfigFileContent (which is public, I assume?) calls into matchFiles which throws without realpath.

[sheetalkamat]

parseJsonSourceFileContent does not directly call into matchFiles. It calls host.readDirectory

[sheetalkamat]

host.readDirectory is either api user specified or uses ts.sys.readdirectory and that uses match files and provides correct realPath to the function. The issue mentioned by @DanielRosenwasser specifically mentions that they use matchFiles which is internal function.

[weswigham]

Ah, yep, I was looking farther down the call stack, it just ping pongs back and forth between ts and the plugin a bit.

[wycats]

@sheetalkamat it seems like @krisselden thinks we really need to use this internal API, which may suggest it should be made public? tildeio/broccoli-typescript-compiler#79 (comment)

[krisselden]

I meant to put this comment here tildeio/broccoli-typescript-compiler#79 (comment)

[wycats]

Could we consider making matchFiles a public API?

[weswigham]

matchFiles is effectively just an internal glob parsing implementation, since we don't want to have runtime dependencies... couldn't you just use any off the shelf globber, like minimatch to implement the same behavior?

[DanielRosenwasser]

I'm honestly a little surprised that we ship matchFiles, and then require a something equivalent to matchFiles as part of our API, but don't let consumers use matchFiles. Forcing a user to reimplement this thing means they have to risk divergence with our implementation.

[DanielRosenwasser]

To be explicit about what I mean, I'm not just stating that I'm surprised. I'm suggesting that we either expose matchFiles, or make it optional and somehow fall back to our internal function.

[weswigham]

To be explicit about what I mean, I'm not just stating that I'm surprised. I'm suggesting that we either expose matchFiles, or make it optional and somehow fall back to our internal function.

We just expect readDirectory to essentially support globs. We don't expose matchFiles because it's an internal version of glob parsing that we'd rather not have other people use - using a more widely used globber would honestly be preferable, so is what we'd rather our consumers use (at least that's what I remember the justification being at the time).

[RyanCavanaugh]

+1 for exposing matchFiles. I'd feel sorry for any tool trying to exactly match our file globbing logic - things can be quirky and it should be possible for a third party to know the exact set of files we'd match.

[wycats]

@RyanCavanaugh Thanks so much. We'll be really happy to be able to drop this private API usage (and hopefully reduce the chance of accidentally relying on breakage in the future).

[krisselden]

@weswigham you are actually making my point for me. People expect us to match the tsc behavior, so if other host implementers should use a "widely used globber," then tsc and tsserver should as well.

People learn what works in a tsconfig.json essentially making the internal thing publicly exposed through the behavior people learn that works quirks and all.

As a host implementer, I don't want to use private API, but there was no reasonable way to match the behavior people associate with configuring TypeScript which was different than a "widely used globber."

I do admit this behavior has become less quirky over time but globbing doesn't actually have an agreed upon specification so it is hard to even define expected behavior vs quirk but any host implementer will get a bug report if something works in tsc and doesn't in their implementation.

[weswigham]

@DanielRosenwasser you wanna merge this or wait for them to be fixed or what?

[tomdale]

@weswigham @RyanCavanaugh @DanielRosenwasser Just to clarify—am I reading the thread correctly that the plan is to expose matchFiles? Is that being tracked here or in a separate issue?

[weswigham]

We had differing opinions on it within the team and didn't really come to a conclusion, re: exposing it. First, we're not terribly happy someone's taken a dependence on our internals, when the original idea behind leaving that part private was that the community at large could likely use a better package for that part when they hosted the compiler (ie, any fully featured globbing package) as we actually originally resisted supporting globs in the first place. Second, if we do expose this publicly, we'd want to change the API to be much more user-friendly (ideally a drop-in replacement for globby or something). So what's internal right now being directly exposed probably isn't going to happen.

[tomdale]

We are similarly not terribly happy that TypeScript exposed custom globbing behavior in tsconfig.json that diverges from fully featured globbing packages. 😉

If TypeScript switched to using globby, minimatch, etc. for processing globs, we would be eager to follow suit. Otherwise, what do you recommend we do to preserve parity with the TypeScript compiler without depending on internals?

[DanielRosenwasser]

I just opened up #34545 where we can continue the discussion (including technical concerns), but it's pretty clear to me that "use a different globber" is a non-option for API consumers.

[sandersn]

@weswigham If the user tests pass after I merged with master, this is technically ready to go. Do you think it's worthwhile with so many errors? It looks like there are correct compiles in there with the failed tests.

@typescript-bot user test this

[typescript-bot]

Heya @sandersn, I've started to run the parallelized community code test suite on this PR at 8c6fe15. You can monitor the build here. It should now contribute to this PR's status checks.

[typescript-bot]

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

[sandersn]

The user test suite run doesn't include broccoli-typescript-compiler. Hmph. @weswigham do you know if I'm doing something wrong here?

[weswigham]

The diff disappears and the build is marked as "failed" when the pre-run build steps failed, which they did. It looks like our swap to the "merge" ref broke this build (unlike every other user test, these guys have a vendored copy of our repo they checkout, which we have to update), since the ref sha doesn't exist unless you explicitly fetch the merge ref; so this test needs a bit of a redesign to accommodate that change of ref.

But on the build itself: I don't like the idea of blessing or testing against a project that utilizes a non-public TS API, so I'm not particularly comfortable with actually adding it. Because of the private API usage, any "breaks" in it are suspect and need extra investigation time to tell if they're expected or not; I don't like it. I think if you use an internal API, it is 100% on you to work around us if we change, and philosophically, we should not give a lick about who is broken by those changes.

[sandersn]

@weswigham OK, I'll close this until we figure out #34545.