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

feat: profile option #201

Merged

Conversation

jason-ha
Copy link
Contributor

@jason-ha jason-ha commented Sep 3, 2024

Add profile option for classes of resolution (and rules) support.
Via config: profile
Via cli: --profile

Currently profiles only list resolutions that are ignored.
Four profiles added:

  • strict - same as before; all resolutions
  • node16 - ignores node10 resolution failures
  • esm-only - ignores CJS resolution failures
  • node16-only - strictly node16 resolutions

Analysis is unchanged. Rendered output (excluding json) and exit codes reflect resolutions ignored by profile. Defects (and general results) are qualified by "(ignored)" or similar when they are not required by profile.

Resolves: #112 Option to skip validation for Node10

Copy link

changeset-bot bot commented Sep 3, 2024

🦋 Changeset detected

Latest commit: 8f9afe0

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

This PR includes changesets to release 2 packages
Name Type
@arethetypeswrong/core Minor
@arethetypeswrong/cli 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

@Maple-Shade
Copy link

Any update on this?

@andrewbranch
Copy link
Collaborator

This PR doesn’t implement anything like the direction I said I’d like to go in #112 (comment), isn’t tested, and doesn’t have any effect on the exit code. Since I don’t have a ton of time to commit to this myself, I’m willing to accept a contribution, but it will have to be a contribution where we collaborate on speccing out exactly what the desired behavior is.

@jason-ha
Copy link
Contributor Author

This PR doesn’t implement anything like the direction I said I’d like to go in #112 (comment),

Is true. This is proposal to resolve the most immediate request that I and others have run into. The recommendation for profiles is not hashed out. It would be possible to build that support on top of these changes. I am unsure of the value gained from just doing profiles to specific things curated versus having the option of full control. Middle ground maybe to have a custom profile option that enables ugly ignore to options.?

isn’t tested,

I locally modified testing but was getting a bunch of what seemed unrelated failures. I then tried testing without any changes and got similar results. This is on Windows system. I don't know what exactly I should expect and it looked too gnarly to investigate. I ran into other problems trying to use repo as configured and expect the config is not robust. For example, pnpm engine versus lockfile. So, I left out any test changes and hoped for some collaboration if there is any value here. I probably should have started a comment thread in here.

and doesn’t have any effect on the exit code.

Well, apparently these changes are garbage... Clearly, we need things to be tested. I thought I manually confirmed a good exit result, but we shouldn't trust humans (I know I fail when I trust myself too often.) Looking at the code I can see a way to adjust.

Since I don’t have a ton of time to commit to this myself, I’m willing to accept a contribution, but it will have to be a contribution where we collaborate on speccing out exactly what the desired behavior is.

  1. I can try a minimal first profile set as listed in referenced comment.
  2. The analysis is still doing all of the checks. okay on that front.
  3. Outputs need adjusted:
    1. Table render should be able to change from hiding to list as failures ignored.
    2. I think we leave JSON alone. Agreed?
    3. Exit code is broken and can be fixed.

I don't know that I can handle test updates though. Maybe I need Linux + node 20+. My Ubuntu is on 18 and there appears to be an issue with nodejs.org today preventing me from even trying 20. Currently I have v18 (which root package.json claims is okay). Below is my attempt to install, build, and test on fresh Ubuntu clone:

> pnpm i --frozen-lockfile
Scope: all 5 workspace projects
Lockfile is up to date, resolution step is skipped
Packages: +495
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Progress: resolved 495, reused 495, downloaded 0, added 495, done

devDependencies:
+ @changesets/cli 2.27.1
+ prettier 3.0.3
+ typescript 5.6.1-rc
> pnpm build
> pnpm tsc && pnpm -r build


> arethetypeswrong@ tsc /home/username/GitHub/attw
> tsc -b

Scope: 4 of 5 workspace projects
packages/web build$ vite build
│ vite v4.4.11 building for production...
│ transforming...
│ ✓ 120 modules transformed.
│ rendering chunks...
│ computing gzip size...
│ ../dist/index.html                     3.04 kB │ gzip:  1.07 kB
│ ../dist/assets/worker-f491c5b6.js  3,603.84 kB
│ ../dist/assets/index-d46c8f80.css      4.09 kB │ gzip:  1.51 kB
│ ../dist/assets/index-35679bb4.js     107.96 kB │ gzip: 36.03 kB
│ ✓ built in 6.16s
└─ Done in 6.3s
> pnpm test
> pnpm -r test

Scope: 4 of 5 workspace projects
packages/cli test$ tsc -b test && node --test 'test/dist/**/*.test.js'
│ Could not find '/home/jasonha/GitHub/attw/packages/cli/test/dist/**/*.test.js'
└─ Failed in 140ms at /home/jasonha/GitHub/attw/packages/cli
packages/core test$ tsc -b test && node --test 'test/dist/**/*.test.js'
│ Could not find '/home/jasonha/GitHub/attw/packages/core/test/dist/**/*.test.js'
└─ Failed in 134ms at /home/jasonha/GitHub/attw/packages/core
/home/jasonha/GitHub/attw/packages/core:
 ERR_PNPM_RECURSIVE_RUN_FIRST_FAIL  @arethetypeswrong/core@0.15.1 test: `tsc -b test && node --test 'test/dist/**/*.test.js'`
Exit status 1

@jason-ha
Copy link
Contributor Author

I figured out the test running limitations. The test command syntax is using glob which requires node version 22. The execution errors then stop on Windows, but no tests are run. With node 22 I was able to fully run tests on Ubuntu.
A PR to clarify the development requirements: #213

Add option to ignore resolution modes such as `Node10`.
Via config: `ignoreResolutions`
Via cli: `--ignoreResolutions`

Option to skip validation for Node10 arethetypeswrong#112
Add four profiles:
- `strict` - same as today; all resolutions
- `node16` - ignores node10 resolution failures
- `esm-only` - ignores CJS resolution failures
- `node16-only` - strictly node16 resolutions
and add profile test cases

Defects (and general results) ignored because of resolution being ignored by profile are shown with "(ignored)" qualifier.
@jason-ha jason-ha force-pushed the ignore-resolutions-option branch from acca169 to 2378133 Compare September 22, 2024 18:43
@jason-ha jason-ha changed the title feat: ignore resolutions option feat: profile option Sep 22, 2024
@andrewbranch
Copy link
Collaborator

FYI, I’ll be on vacation without a computer until mid-October. This may take some time for me to get to, but I’m excited about this work! Thanks for your patience!

@jason-ha
Copy link
Contributor Author

@andrewbranch, how was your vacation? Just adding a little nudge for review of this work.

Copy link
Collaborator

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

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

Sorry for the delay! Let’s get this merged soon 👍

"esm-only": {
ignoreResolutions: ["node10", "node16-cjs"],
},
"node16-only": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we remove this one? I can’t think of a good reason why something should pass node16 but fail bundler.

Copy link
Contributor Author

@jason-ha jason-ha Nov 8, 2024

Choose a reason for hiding this comment

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

It will be your call. I don't know when bundler might not work for something. I would expect that there are some packages that never intend to be bundled and "node16-only" lets that intent be specified.

packages/cli/README.md Outdated Show resolved Hide resolved
@andrewbranch andrewbranch merged commit 658982e into arethetypeswrong:main Nov 10, 2024
2 checks passed
@jason-ha jason-ha deleted the ignore-resolutions-option branch November 13, 2024 03:17
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.

Option to skip validation for Node10
3 participants