-
Notifications
You must be signed in to change notification settings - Fork 14
Add support for multiple subprojects #16
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,8 +2,8 @@ import { spawn, SpawnOptions } from "child_process"; | |||||||||
| import type { Plugin as VitePlugin } from "vite"; | ||||||||||
|
|
||||||||||
| // Utility to invoke a given sbt task and fetch its output | ||||||||||
| function printSbtTask(task: string, cwd?: string): Promise<string> { | ||||||||||
| const args = ["--batch", "-no-colors", "-Dsbt.supershell=false", `print ${task}`]; | ||||||||||
| function printSbtTasks(tasks: Array<string>, cwd?: string): Promise<Array<string>> { | ||||||||||
| const args = ["--batch", "-no-colors", "-Dsbt.supershell=false", ...tasks.map(task => `print ${task}`)]; | ||||||||||
| const options: SpawnOptions = { | ||||||||||
| cwd: cwd, | ||||||||||
| stdio: ['ignore', 'pipe', 'inherit'], | ||||||||||
|
|
@@ -28,24 +28,68 @@ function printSbtTask(task: string, cwd?: string): Promise<string> { | |||||||||
| if (code !== 0) | ||||||||||
| 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)); | ||||||||||
| }); | ||||||||||
| }); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| export interface Subproject { | ||||||||||
| projectID: string | null, | ||||||||||
| uriPrefix: string, | ||||||||||
| } | ||||||||||
|
|
||||||||||
| export interface ScalaJSPluginOptions { | ||||||||||
| cwd?: string, | ||||||||||
| projectID?: string, | ||||||||||
| uriPrefix?: string, | ||||||||||
| subprojects?: Array<Subproject>, | ||||||||||
| } | ||||||||||
|
|
||||||||||
| export default function scalaJSPlugin(options: ScalaJSPluginOptions = {}): VitePlugin { | ||||||||||
| const { cwd, projectID, uriPrefix } = options; | ||||||||||
| 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") | ||||||||||
|
||||||||||
| 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit undecided there. Sure, this can be the way. OTOH, I suspect those variables might be needed soon.
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -18,6 +18,23 @@ function normalizeSlashes(path: string | null): string | null { | |||||||||||||||
| return path === null ? null : path.replace(/\\/g, '/'); | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| function testBothModes( | ||||||||||||||||
| testFunction: (d: string, func: () => Promise<void>, options: TestOptions) => void, | ||||||||||||||||
| description: string, | ||||||||||||||||
| f: (mode: string, suffix: string) => Promise<void>, | ||||||||||||||||
| testOptions: TestOptions, | ||||||||||||||||
| ) { | ||||||||||||||||
| testFunction ||= it | ||||||||||||||||
|
||||||||||||||||
| const MODES = [["production", MODE_PRODUCTION, "opt"], ["development", MODE_DEVELOPMENT, "fastopt"]] | ||||||||||||||||
| MODES.forEach( ([modeName, mode, suffix]) => { | ||||||||||||||||
| testFunction( | ||||||||||||||||
| description + "(" + modeName + ")", | ||||||||||||||||
|
||||||||||||||||
| description + "(" + modeName + ")", | |
| description + " (" + modeName + ")", |
will probably display better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| it.fails("when both projectID and subproojects are specified", async () => { | |
| it.fails("when both projectID and subprojects are specified", async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| it.fails("when both uriPrefix and subproojects are specified", async () => { | |
| it.fails("when both uriPrefix and subprojects are specified", async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is subprojects: [] something that should be supported at all? Probably not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you quite sure this always works? Can't we get info lines between the two
printresults?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.