Merge turf:master into experimental dx branch #2682
Conversation
…ckage, rather than specifying the arguments on the command line.
…facts. See discussion Turfjs#2629
…o building to work. Simplifies things overall.
…lined in dist/esm/index.js files, greatly increasing their size.
…re causing problems in a previous build because skipNodeModulesBundle was true. That's now been removed and @turf/turf does build fine without them.
|
Wow this is actually insanely fast timings on an M1 pro (to run the @turf/difference tests) |
|
I spent some time trying to get typescript project references to work, but I think there's some weird issue with rollup/tsup and project references that I don't fully understand. |
| "prepare": "lerna run build && husky install", | ||
| "test": "pnpm run lint && lerna run test && lerna run --scope @turf/turf last-checks" | ||
| "prepublish": "lerna run build && lerna run --scope @turf/turf last-checks", | ||
| "test": "lerna run test" |
There was a problem hiding this comment.
Just a couple thoughts here:
- it dropped
pnpm run lint - I don't remember exactly why but I think last-checks depends on
lerna run test?
What do you think about making a new script entry:
`"ci": "lerna run build && lerna run lint && lerna run test && lerna run --scope @turf/turf last-checks"
And then just calling this new script explicitly in the .github/*.yml files?
We could also break build / lint / (test & last checks) into 3 different scripts for more granular timings on CI. In general I think we can get away from some of the magic script names that make pnpm do things at certain lifecycle moments.
There was a problem hiding this comment.
I looked into this some more and last-checks only depends on last-checks:testjs to run before last-checks:example and it doesn't actually depend on the other packages, so that seems safe to move. I'm not actually sure why it lives in its own step, I'll make a PR cleaning that up.
There was a problem hiding this comment.
#2690 cleans up some last-checks
There was a problem hiding this comment.
I think I'm leaning towards the following:
package.json scripts:
"build": "lerna run build",
"lint": "lerna run lint",
"test": "lerna run test"
github workflows:
run: pnpm build added after run: pnpm install --frozen-lockfile everywhere
run: pnpm lint added before run: pnpm test everywhere
This also will cause us to need to make changes in the CONTRIBUTING.md file
Local Setupno longer requirespnpm testDevelopment tipsandPull Request Checklistcan probably also recommend runningpnpm lint
| @@ -28,7 +28,7 @@ | |||
| "{projectRoot}/test/**", | |||
| "{projectRoot}/types.ts" | |||
| ], | |||
| "dependsOn": ["build"], | |||
| "dependsOn": [], | |||
There was a problem hiding this comment.
I will make a follow-up PR to sort out some caching issues. Unless you're executing tasks with pnpm lerna run test --scope=@turf/pkg this file doesn't come into play though, and CI doesn't cache anything so we're safe there as well.
There was a problem hiding this comment.
#2685 is the change to the inputs to fix test caching. I intentionally didn't remove the build dependsOn entry because you're doing that here and I didn't want to conflict.
|
Closing this PR as it is unnecessary. This was just to pull the latest updates from master into my experimental branch. However could have just done that from master in my fork. |
Closing this PR as it is unnecessary. This was just to pull the latest updates from master into my experimental branch. However could have just done that from master in my fork.
Will create another more traditional PR to review and pull changes from experimental branch into master.