Initial implementation. #1
Conversation
sjrd
force-pushed
the
initial-implementation
branch
from
b8ca73d to
6a9a3d3
Compare
|
@gzm0 Do you think you might have some time for this PR in the coming week? Should I ask someone else to review? Does this plugin make sense to you overall, in terms of what it offers? |
|
Sorry, I missed this over the weekend. Given the current forecast for the weekend, I'm quite confident I can find the time to review this. Is that enough? |
😄 Yes, sure, that's enough. Thank you! |
sjrd
force-pushed
the
initial-implementation
branch
2 times, most recently
from
37335b4 to
440d099
Compare
|
As an additional confidence test, here is a "real-world" usage of the plugin: scalacenter/scastie#790 |
package.json
Outdated
| }, | ||
| "dependencies": { | ||
| "vite": "^4.1.4", | ||
| "vitest": "^0.29.2" |
There was a problem hiding this comment.
Are you sure this shouldn't go into the devDependencies?
There was a problem hiding this comment.
No, indeed. I moved it to devDependencies.
tsconfig.json
Outdated
| "./*.ts" | ||
| ], | ||
| "exclude": [ | ||
| "__test__/**.ts", |
There was a problem hiding this comment.
What generates this directory? vitest?
There was a problem hiding this comment.
Cargo-cult copy-paste. :p I removed it.
index.ts
Outdated
|
|
||
| // standard Rollup | ||
| resolveId(source, importer, options) { | ||
| const parts = /^scalajs:(.*)$/g.exec(source); |
There was a problem hiding this comment.
Consider making the prefix configurable. Otherwise it's impossible to use this plugin with multiple sbt projects (not a core use-case of course, but it seems very inconsistent to me otherwise).
There was a problem hiding this comment.
I added a uriPrefix configuration for this.
tsconfig.json
Outdated
| }, | ||
| "include": [ | ||
| "./*.ts" | ||
| ], |
There was a problem hiding this comment.
Consider leaving the default (include: ["**/*"]).
| @@ -0,0 +1,47 @@ | |||
| name: Node.js test and Build | |||
There was a problem hiding this comment.
This doesn't seem to actually run...
There was a problem hiding this comment.
It will run once the workflow hits main. It's always the same issue when we bootstrap a project.
I linked a green build of the same commit in my sjrd/* repo: https://github.com/sjrd/vite-plugin-scalajs/actions/runs/4376164277
| return path === null ? null : path.replace(/\\/g, '/'); | ||
| } | ||
|
|
||
| describe("scalaJSPlugin", () => { |
There was a problem hiding this comment.
Add a test case that sets isDev to true?
index.ts
Outdated
| export default function scalaJSPlugin(options: ScalaJSPluginOptions = {}): VitePlugin { | ||
| const { projectID, cwd } = options; | ||
|
|
||
| let isDev = false; // when resolving the Vite config, this may turn into true |
There was a problem hiding this comment.
Consider being stricter here and type isDef: boolean | undefined. Then fail if configResolved didn't get called.
index.ts
Outdated
| const { projectID, cwd } = options; | ||
|
|
||
| let isDev = false; // when resolving the Vite config, this may turn into true | ||
| let scalaJSOutputDir: string; |
There was a problem hiding this comment.
Same here actually: Don't resolve to the empty string if you didn't get the callbacks you expected.
sjrd
force-pushed
the
initial-implementation
branch
from
5e97fa9 to
f460a9a
Compare
sjrd
force-pushed
the
initial-implementation
branch
from
f460a9a to
95b4d9a
Compare
|
Thanks for the review! I believe I have addressed all the comments. |
This plugin can be used as follows (or will be, once published):
sjrd/scalajs-sbt-vite-laminar-chartjs-example@getting-started-tutorial-series...vite-plugin
This is as a follow-up to scala-js/scala-js-website#590 (comment)
Regarding tests: they directly interact with the plugin, without going through Vite or Rollup. This seems to be standard practice among all the Vite and/or Rollup plugins I looked at. Apparently, integration tests (like with have with sbt
scripted) are not a thing in that universe.