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 #4398

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

monyedavid
Copy link
Contributor

@monyedavid monyedavid commented Jan 24, 2025

This pr implements the examples for jslib/dependencies.

#3927

Checklist:

  • example/jslib/linting
    • 1-linting
    • 2-autoformatting
    • 3-code-coverage

Key changes:

  • Add coverage for Jest, Jasmine, Mocha and Vitest.
  • Add lint and auto format support with prettier and eslint.
  • Generate test config for Jest & Vite, with option to use a custom test config.

@lihaoyi
Copy link
Member

lihaoyi commented Jan 27, 2025

The docs for Linting and AutoFormatting with Eslint and Prettier appears to be duplicated twice

Linting Typescript Projects __ The Mill JVM Build Tool.pdf

@lihaoyi
Copy link
Member

lihaoyi commented Jan 27, 2025

Is there a reason example/javascriptlib/testing/1-test-suite/vite.config.ts and all the other files was deleted?

@monyedavid
Copy link
Contributor Author

monyedavid commented Jan 27, 2025

Is there a reason example/javascriptlib/testing/1-test-suite/vite.config.ts and all the other files was deleted?

yes, mill generates by default those files... but you can opt to use yours. I will update the testing docs.

@monyedavid
Copy link
Contributor Author

The docs for Linting and AutoFormatting with Eslint and Prettier appears to be duplicated twice

Linting Typescript Projects __ The Mill JVM Build Tool.pdf

will clean out the auto-format docs

@@ -1,23 +0,0 @@
import {pathsToModuleNameMapper} from 'ts-jest';
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason these files were deleted?

}
}

> mill foo.checkFormatAll # run linter - since both eslint and prettier configurations are present, mill will opt to use eslint.
Copy link
Member

Choose a reason for hiding this comment

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

Let's move the default checkFormatAll command to an ExternalModule, similar to ScalafmtModule/PalantirFormatModule/KtlintModule. That will help standardize the user experience across Mill language toolchains

Checking formatting...
[warn] foo/src/foo.ts
[warn] Code style issues found. Run foo.reformatAll to fix.
*/
Copy link
Member

Choose a reason for hiding this comment

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

This example should focus on non-autoformat-able lint errors, as the 2-autoformatting/ section already covers those. This section should also demonstrate that fixing the error (e.g. using sed) makes the lint errors go away.

@@ -1,31 +0,0 @@
import {pathsToModuleNameMapper} from 'ts-jest';
Copy link
Member

Choose a reason for hiding this comment

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

Why was this deleted?

}

// web browser - serve coverage report
def htmlReport: T[Unit] = Task {
Copy link
Member

@lihaoyi lihaoyi Jan 28, 2025

Choose a reason for hiding this comment

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

Is there a way to just return a HTML file on disk rather than serving it in a web browser?

os.symlink(Task.workspace / "out/.nycrc", compile()._1.path / ".nycrc")
}

def nycrcBuilder: Task[Path] = Task.Anon {
Copy link
Member

Choose a reason for hiding this comment

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

This task is very weird. It seems like it should be a T[PathRef] returning the config file it writes

Copy link
Member

Choose a reason for hiding this comment

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

Also what's nycrc? Maybe use a longer name or add a comment?


// <rootDir> = '/out'; allow coverage resolve distributed source files.
// & define coverage files relative to <rootDir>.
private[TestModule] def link: Task[Unit] = Task.Anon {
Copy link
Member

Choose a reason for hiding this comment

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

This name is far too short given how niche the task is. Maybe call it coverageSetupSymlinks or something?


def getConfigFile: T[String] =
Task { (compile()._1.path / "jest.config.ts").toString }
def conf: Task[Path] = Task.Anon {
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it should be a T[PathRef]

@@ -96,42 +199,116 @@ object TestModule {
runTest()
}

// with coverage
def coverageConf: Task[Path] = Task.Anon {
Copy link
Member

Choose a reason for hiding this comment

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

T[PathRef]

File...| % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s...
---------------|---------|----------|---------|---------|-------------------
...All files...|...100 |...100 |...100 |...100 |...
...calculator.ts...|...100 |...100 |...100 |...100 |...
Copy link
Member

Choose a reason for hiding this comment

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

At least one of the code coverage examples should have <100% coverage so we can check that that behaves as expected

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.

2 participants