Skip to content
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

Add first class Javascript/Typescript support to the Mill build tool #4003

Merged
merged 39 commits into from
Dec 5, 2024

Conversation

monyedavid
Copy link
Contributor

@monyedavid monyedavid commented Nov 21, 2024

Related Issues

#3927

Checklist

  • example/jslib/basic
    • 1-simple/: A minimal Typescript module demonstrating typechecking/running/testing/bundling of a Node CLI tool
    • 2-react/: A minimal Typescript module demonstrating typechecking/running/testing/bundling of a React-based web application, served using a static HTML page
    • 3-custom-build-logic/: A Typescript module with custom build logic

	- running
	- bundling of a Node CLI tool
	- type checking
	- testing
@monyedavid monyedavid marked this pull request as draft November 21, 2024 14:58
@lihaoyi
Copy link
Member

lihaoyi commented Nov 22, 2024

Each top-level bullet of the original bounty should be its own PR. For this PR, just focus on example/jslib/basic/, and we can review/merge/payout the first part of the bounty first. Extending it for further top-level bullets can come later

@monyedavid monyedavid marked this pull request as ready for review November 25, 2024 15:48
@monyedavid monyedavid requested a review from lihaoyi November 25, 2024 16:41
@monyedavid monyedavid requested a review from lihaoyi November 26, 2024 20:50
@lihaoyi
Copy link
Member

lihaoyi commented Nov 27, 2024

It's a start and good that tests pass, but the code in the various build.mill example projects is far too messy to serve their purpose as documentation examples.

For reference, look at the equivalent build.mill files in example/pythonlib/basic/ or example/kotlinlib/basic/ for what I would expect the build.mill files to look like. Everything else should be pushed upstream into javascriptlib/src/ library traits and helpers, and those will have to be organized appropriately (see pythonlib/src/ as an example)

- Abstract Jest Testing
- Abstract React scripts
 - allow for specification of compiler options
@lihaoyi
Copy link
Member

lihaoyi commented Nov 30, 2024

@jodersky could you give this a review when you have some time? Since your work on the Python side is very similar you may have ideas that are transferable to JS/TS

refine TypeScriptModule:
 - user defined compiler options
refine ReactScriptsModule:
 - user defined compiler options
 - user defined package options
 - user defined env options
RsWithServeModule:
 - serve static html files via serve.js
@monyedavid monyedavid requested a review from lihaoyi November 30, 2024 12:16
- use `Task[(Option[Seq[String]], Option[Seq[String]])]` for computed arguments
fix failing tests
@jodersky
Copy link
Member

Will do!

 - update default tsconfig
 - use ts-node to run
 - use esbuild script to compile ts
 - compilerOptions can be modified from calling object
- include declarations of module dependencies in paths
- correct path to typescript
- add bundle flags
add missing comma to bundle script
Copy link
Member

@jodersky jodersky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be easier to review and improve if each example were its own pull request, starting with only the basic example.

A couple of things stand out to me:

  • I think that testing can be factored out into its own module, without having to subclass modules and have custom tasks. Eg. instead of defining the "main" module to be a JestModule with testSources etc, the test sources could become the normal sources of a nested JestModule which would extend something like `TestModule. This is how Mill's Java and Scala support does it, and is what I used for the Python support as well.

  • It will also be easier to approach this by documentation to examples first, and also commenting what every task is supposed to do.

In general, I'd suggest to take a look at what we're doing with pythonlib and use it as inspiration on how to approach this. The reason is that it's still fairly minimal and to-the-point, but it does strive to follow general tendencies and structure of the established javalib and scalalib.

@monyedavid
Copy link
Contributor Author

monyedavid commented Dec 3, 2024

I think this would be easier to review and improve if each example were its own pull request, starting with only the basic example.

A couple of things stand out to me:

  • I think that testing can be factored out into its own module, without having to subclass modules and have custom tasks. Eg. instead of defining the "main" module to be a JestModule with testSources etc, the test sources could become the normal sources of a nested JestModule which would extend something like `TestModule. This is how Mill's Java and Scala support does it, and is what I used for the Python support as well.
  • It will also be easier to approach this by documentation to examples first, and also commenting what every task is supposed to do.

In general, I'd suggest to take a look at what we're doing with pythonlib and use it as inspiration on how to approach this. The reason is that it's still fairly minimal and to-the-point, but it does strive to follow general tendencies and structure of the established javalib and scalalib.

the abstraction (TestModule) is done in another pr, as that extends beyond the scope of this task set (and many other things here really)

I can open that next after this is done / move them here whichever you prefer @jodersky

Separating into smaller pr's would be somewhat difficult, its length is due to the modifications made to TypescriptModule and creating the other two modules. I promise the other pr's would be much smaller.

Also as there are different Test frame works I will be working with through the project,
we have a TestModule extends TypeScriptModule, which contains core functionality for its extenders.
JestModule, MochaModule and others extend TestModule, making it easier to use and reducing duplicate code

@monyedavid monyedavid requested a review from jodersky December 3, 2024 18:13
@lihaoyi
Copy link
Member

lihaoyi commented Dec 4, 2024

I think this looks in decent enough state to merge. It's not perfect, but it would be useful to get some of the follow on use cases in place first before polishing further.

@monyedavid there's a test failure that needs to be fixed, then I'll merge this and close out the bounty

@lihaoyi lihaoyi merged commit 50deaaa into com-lihaoyi:main Dec 5, 2024
27 checks passed
@lihaoyi
Copy link
Member

lihaoyi commented Dec 5, 2024

Merged this, thanks @monyedavid for bearing with all our review and feedback! I think the PR is in a great state now to build upon for later, and I expect follow-up PRs to be easier since you are now more familiar with the Mill tool, codebase, and coding conventions.

Please send me your international bank transfer details over email and I will wire over the bounty

@lefou lefou added this to the 0.12.4 milestone Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants