Conversation
…in bundling. Added diagnostics and more tests
| public expectToHaveDiagnostics(): this { | ||
| expect(this.getLuaDiagnostics()).toHaveDiagnostics(); | ||
| public expectToHaveDiagnostic(matcher: DiagnosticMatcher): this { | ||
| expect(this.getLuaDiagnostics().find(matcher)).toBeDefined(); |
There was a problem hiding this comment.
Is that really something we'd need that often to add a new method there?
There was a problem hiding this comment.
We don't really have an alternative if we don't want to specify the exact diagnostic each time (which looks really messy and is not really required imo).
There was a problem hiding this comment.
I'd rather use diagnostic codes for that. In the future we should probably use an enum, or copy TypeScript's diagnostic messages concept to avoid magic numbers.
| public expectToHaveDiagnostics(): this { | ||
| expect(this.getLuaDiagnostics()).toHaveDiagnostics(); | ||
| public expectToHaveDiagnostic(matcher: DiagnosticMatcher): this { | ||
| expect(this.getLuaDiagnostics().find(matcher)).toBeDefined(); |
There was a problem hiding this comment.
I'd rather use diagnostic codes for that. In the future we should probably use an enum, or copy TypeScript's diagnostic messages concept to avoid magic numbers.
|
|
||
| type CommandLineOption = CommandLineOptionOfEnum | CommandLineOptionOfBoolean; | ||
| interface CommandLineOptionOfString extends CommandLineOptionBase { | ||
| type: "string"; |
…ePath and rootDir, addressed other PR comments
src/bundle.ts
Outdated
| const diagnostics: ts.Diagnostic[] = []; | ||
|
|
||
| const resolvedEntryModule = resolveFromRootDir(program, entryModule); | ||
| if (!transpiledFiles.some(f => resolveFromRootDir(program, f.fileName) === resolvedEntryModule)) { |
There was a problem hiding this comment.
Is resolveFromRootDir required? Isn't fileName always resolved and normalized?
There was a problem hiding this comment.
Don't think it is when you manually construct the program (like we do in our tests)
| @@ -0,0 +1,18 @@ | |||
| import * as path from "path"; | |||
There was a problem hiding this comment.
We have few tests with tsconfig.json in CLI, but yeah, we don't have any tests using transpileProject API. Maybe at least we may reduce scope of that test? I.e. rename it to project and remove that imaginary host API (which we already test)?
ark120202
left a comment
ark120202
left a comment
There was a problem hiding this comment.
Other than these 3 small notes LGTM. There are still few unresolved things but I'm fine with doing them in follow-up PRs
# Conflicts: # src/utils.ts
Perryvw
force-pushed
the
feature/bundling-v2
branch
from
7fc3b28 to
5003460
Compare
| const requireOverride = ` | ||
| local ____moduleCache = {} | ||
| local ____originalRequire = require | ||
| function require(file) |
There was a problem hiding this comment.
Just curious, is there a specific reason for this not being local? It would still equivalently be used by the whole file, but with the benefit of not leaking outside this file
There was a problem hiding this comment.
No specific reason, we did not consider this at all. I think it is indeed a good idea to make this adjustment.
3 participants
Alternative approach to bundling from #648 and #747.
Took test cases from #648.
Instead of using outFile, instead uses
luaBundleandluaBundleEntrytstl options.