Add support for multiple subprojects#16
Conversation
|
The failure on Windows is likely related to argument quoting/escaping. Maybe I should get a Windows VM and debug it there. |
sjrd
left a comment
sjrd
left a comment
There was a problem hiding this comment.
Thanks for the PR. This should be good to have, indeed.
I have some comments, but I'm mostly concerned about the comment in printSbtTasks.
| f: (mode: string, suffix: string) => Promise<void>, | ||
| testOptions: TestOptions, | ||
| ) { | ||
| testFunction ||= it |
There was a problem hiding this comment.
This like does not seem necessary.
| function extractSubprojects(options: ScalaJSPluginOptions): Array<Subproject> { | ||
| if (options.subprojects) { | ||
| if (options.projectID || options.uriPrefix) { | ||
| throw new Error("If you specify subprojects, you cannot specify projectID / uriPrefix") |
There was a problem hiding this comment.
Coding style: we use semicolons in this codebase:
| throw new Error("If you specify subprojects, you cannot specify projectID / uriPrefix") | |
| throw new Error("If you specify subprojects, you cannot specify projectID / uriPrefix"); |
This comment applies everywhere.
| reject(new Error(`sbt invocation for Scala.js compilation failed with exit code ${code}.`)); | ||
| else | ||
| resolve(fullOutput.trimEnd().split('\n').at(-1)!); | ||
| resolve(fullOutput.trimEnd().split('\n').slice(-tasks.length)); |
There was a problem hiding this comment.
Are you quite sure this always works? Can't we get info lines between the two print results?
There was a problem hiding this comment.
Maybe we can. I am not sure what to do there, though. We could filter lines starting with [info] (and warning etc.) and assume that this is not the actual path. (Technically, many weird characters might occur there, IIRC Linux allows everything but '\0' and '/' in a file name. I am sure newlines would probably break it, too, so it depends how much we want to be perfectionists…)
Other alternative would be calling SBT twice, but this would increate startup time, as starting two SBT instances in parallel sometimes fails. This still wouldn't resolve the issue with newlines, so maybe filtering [info] and similar prefixes would be a better way.
| const spByProjectID = mapBy(subprojects, (p) => p.projectID, "projectID") | ||
| const spByUriPrefix = mapBy(subprojects, (p) => p.uriPrefix, "uriPrefix") |
There was a problem hiding this comment.
These variables are unused. So instead of a mapBy, consider simplifying to only check duplicates in an array:
function checkNoDuplicates<T>(a: Array<T>, itemName: string): voidthen call it as
| const spByProjectID = mapBy(subprojects, (p) => p.projectID, "projectID") | |
| const spByUriPrefix = mapBy(subprojects, (p) => p.uriPrefix, "uriPrefix") | |
| checkNoDuplicates(subprojects.map((p) => p.projectID), "projectID"); | |
| checkNoDuplicates(subprojects.map((p) => p.uriPrefix), "uriPrefix"); |
There was a problem hiding this comment.
I am a bit undecided there. Sure, this can be the way. OTOH, I suspect those variables might be needed soon.
| const MODES = [["production", MODE_PRODUCTION, "opt"], ["development", MODE_DEVELOPMENT, "fastopt"]] | ||
| MODES.forEach( ([modeName, mode, suffix]) => { | ||
| testFunction( | ||
| description + "(" + modeName + ")", |
There was a problem hiding this comment.
Nit:
| description + "(" + modeName + ")", | |
| description + " (" + modeName + ")", |
will probably display better.
| expect(await plugin.resolveId.call(fakePluginContext, 'scalajs/main.js')) | ||
| .toBeNull(); |
There was a problem hiding this comment.
| expect(await plugin.resolveId.call(fakePluginContext, 'scalajs/main.js')) | |
| .toBeNull(); | |
| expect(await plugin.resolveId.call(fakePluginContext, 'scalajs:main.js')) | |
| .toBeNull(); |
would make more sense, wouldn't it?
There was a problem hiding this comment.
Correct. BTW, the scalajs/main.js is copied from other tests. Maybe we should fix it in the other places, although it is out of scope of this PR.
| }); | ||
| }); | ||
|
|
||
| it.fails("when both projectID and subproojects are specified", async () => { |
There was a problem hiding this comment.
| it.fails("when both projectID and subproojects are specified", async () => { | |
| it.fails("when both projectID and subprojects are specified", async () => { |
| }); | ||
| }); | ||
|
|
||
| it.fails("when both uriPrefix and subproojects are specified", async () => { |
There was a problem hiding this comment.
| it.fails("when both uriPrefix and subproojects are specified", async () => { | |
| it.fails("when both uriPrefix and subprojects are specified", async () => { |
| it.fails("when both uriPrefix and subproojects are specified", async () => { | ||
| setup({ | ||
| uriPrefix: "xxx", | ||
| subprojects: [] |
There was a problem hiding this comment.
Is subprojects: [] something that should be supported at all? Probably not.
There was a problem hiding this comment.
Yeah, it is a kind of an edge case.
- Theoretically, one might use it dynamically and no subprojects would equal to disabled plugin.
- Practically, such use case is unlikely to be common.
- It also might require the plugin to handle those edge cases. (Well, I am not sure if printSbtTasks handles empty list well…)
So, I'll disable this edge case.
EDIT: disabled
| }); | ||
|
|
||
|
|
||
| it("does not work with a project that does not link", async () => { |
There was a problem hiding this comment.
Double blank line:
| }); | |
| it("does not work with a project that does not link", async () => { | |
| }); | |
| it("does not work with a project that does not link", async () => { |
2 participants
Adds support for multiple subproject with just single application of the plugin and with just one SBT call. This prevents issues with parallel SBT calls.
Note that function testBothModes can be used for further test simplification.