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

Implement selective typecheck #2321

Merged
merged 6 commits into from
Mar 28, 2023

Conversation

sgb-io
Copy link
Contributor

@sgb-io sgb-io commented Mar 22, 2023

Selective TypeCheck

  • Implements selective typecheck, i.e adds the --ancestors, --descendants, --changed and --compareBranch options to modular typecheck
  • Maintains backwards compatibility with the existing modular typecheck
  • Adds new docs that describe how Modular currently treats tsconfig.json files, plus docs for the new options
  • Updates the tests for typecheck to use a new approach, and cover new cases

New unit testing approach: calling commands directly

A side mission during this PR was to call typecheck() directly from test code, instead of rely on execa as is very common in the project. The execa approach is (a) slow and (b) breaks the tracking of test coverage. typecheck.test.ts demonstrates a new approach:

  • Call the command directly, avoiding the need for a child process, and fixing jest's ability to track test coverage
  • Mock getModularRoot in tests, which is used extensively throughout the project
  • Create a secondary getModularRoot clone called getRealModularRootInTest that is designed to be used in test code, i.e. avoid any mocks that might be applicable
  • Avoid the actionPreflightCheck, which complicates the mocking (it contains a call to getModularRoot) and slows things down. Plus, this check has no value in test (it can be tested in isolation).

It should be noted that I've used jest.doMock which avoids jest's default hoisting behaviour. In other words, it lets us mock the modular root just before importing and calling a modular command. To work with this approach, I dynamically import the typecheck command. It may also be possible to use a more static setup.

Not only is this approach faster and simpler, the coverage for the test command has gone from 0 to >90%.

TODO

  • New docs
  • Repoint PR to v4.3 or main when v4.2 is released
  • Tests
  • Changeset

@changeset-bot
Copy link

changeset-bot bot commented Mar 22, 2023

🦋 Changeset detected

Latest commit: 4ae9bcc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
modular-scripts Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@coveralls
Copy link
Collaborator

coveralls commented Mar 22, 2023

Coverage Status

Coverage: 23.335% (+2.9%) from 20.414% when pulling 4ae9bcc on sgb-io:feat/selective-typecheck into 2c25504 on jpmorganchase:feature/v4.3.

@sgb-io sgb-io changed the base branch from feature/v4.2 to feature/v4.3 March 22, 2023 14:38
@sgb-io sgb-io marked this pull request as ready for review March 23, 2023 16:52
if (tsConfig.compilerOptions && rest.compilerOptions?.jsx) {
tsConfig.compilerOptions.jsx = rest.compilerOptions.jsx;
// Where the user has defined allowlist-supported compiler options, apply them
for (const item of COMPILER_OPTIONS_ALLOW_LIST) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This block is currently uncovered, worth adding a test case for it (essentially, using the jsx option)

@sgb-io sgb-io requested review from AlbertoBrusa and cristiano-belloni and removed request for AlbertoBrusa March 27, 2023 10:07
@@ -56,9 +56,10 @@ async function getPackageMetadata() {
// validate tsconfig
// Extract configuration from config file and parse JSON,
// after removing comments. Just a fancier JSON.parse
const userTsConfigPath = path.join(modularRoot, typescriptConfigFilename);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bugfix. The second argument was previously reading tsconfig.json from the cwd(), since no modular root was being used. This is not consistent with the first argument, where the modular root is used.

That said, in practice, there was no actual way for users to be affected by this bug - since commands are supposed to be run in the root already (so not using the modular root resulted in the correct file being used), and preflight errors actually get in the way first if you try to run something like typecheck from the non-root dir.

The bugfix here is needed so that we can effectively test the code in test, i.e. use a faked modular root and have it respected.

@sgb-io sgb-io merged commit 962b5e5 into jpmorganchase:feature/v4.3 Mar 28, 2023
@sgb-io sgb-io deleted the feat/selective-typecheck branch March 28, 2023 13:27
@github-actions github-actions bot mentioned this pull request Apr 18, 2023
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.

3 participants