feat!: add packageManger information + make jsWorkspace packagePaths relative #4643

lukasholzer · 2022-10-19T15:32:51Z

🎉 Thanks for submitting a pull request! 🎉

Summary

This is needed for https://github.com/netlify/pillar-workflow/issues/895

This is a breaking change as we changed the jsWorkspace paths to be relatives than absolute ones.

The breaking change won't have an influence in buildbots usage as we just check if the property is present. There are no other users of @netlify/build-info at the moment in our organization:

For us to review and ship your PR efficiently, please perform the following steps:

Open a bug/issue before writing your code 🧑‍💻. This ensures
we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or
something that`s on fire 🔥 (e.g. incident related), you can skip this step.
Read the contribution guidelines 📖. This ensures your code follows our style guide and
passes our tests.
Update or add tests (if any source code was changed or added) 🧪
Update or add documentation (if features were changed or added) 📝
Make sure the status checks below are successful ✅

A picture of a cute animal (not mandatory, but encouraged)

packages/headers-parser/package.json

packages/build-info/src/build-info.ts

lukasholzer · 2022-10-21T09:29:00Z

packages/build-info/package.json

-  "main": "./lib/main.js",
-  "types": "./lib/main.d.ts",
+  "exports": {
+    ".": "./lib/index.js"


restrict subpath imports (this should help to not introduce breaking changes that easily)

Only functions that are exported from lib/index.js are public api surface

lukasholzer · 2022-10-21T09:35:47Z

packages/build-info/src/bin.ts

 import { exit, argv } from 'process'

+import yargs, { Arguments } from 'yargs'


This is the entrypoint you should use with ESModules according to the documentation:

Which has a slightly different api on the .command https://github.com/yargs/yargs

lukasholzer · 2022-10-21T09:36:29Z

packages/build-info/src/context.ts

@@ -23,7 +23,7 @@ export type ContextOptions = {

 export type Context = {
  projectDir: string
-  rootDir: string
+  rootDir?: string


The root dir must not be set (could be only projectDir as well in the regular case of being not a mono repository)

lukasholzer · 2022-10-21T09:37:35Z

packages/build-info/src/index.ts

@@ -0,0 +1 @@
+export { getBuildInfo } from './get-build-info.js'


This is now the only public API surface we expose every change to this function signature is considered breaking.

All other functions are not on the API surface (except the behaviour is changed)

packages/build-info/src/bin.ts

packages/build-info/src/detect-package-manager.ts

packages/build-info/tests/test-setup.ts

…relative

packages/build-info/src/detect-package-manager.ts

packages/build-info/src/workspaces.ts

Approved by accident.

jobala · 2022-10-25T07:09:25Z

Assuming a user specifies the NETLIFY_USE_YARN env variable but then specifes pnpm in package.json's packageManager field. What happens then? My guess is that we will try to yarn install which will fail with a This project is configured to use pnpm error.

lukasholzer · 2022-10-25T10:03:52Z

Issue https://github.com/netlify/pillar-workflow/issues/895

lukasholzer · 2022-10-25T10:05:51Z

Assuming a user specifies the NETLIFY_USE_YARN env variable but then specifes pnpm in package.json's packageManager field. What happens then? My guess is that we will try to yarn install which will fail with a This project is configured to use pnpm error.

Yea but we cannot do better there the env variable needs to override (to not break excisting behaviours) but corepack only respects that.

Once this is out we don't need the environment variables anymore.

or what would be your suggestion

github-actions · 2022-10-25T10:07:07Z

This pull request adds or modifies JavaScript (.js, .cjs, .mjs) files.
Consider converting them to TypeScript.

jobala · 2022-10-25T11:04:44Z

or what would be your suggestion

We have enough information to detect the clash between the environment variable and the packageManager field. We could either

Let the user know there's a clash and fail the build
Let the user know there's a clash and that we are defaulting to the what is specified in the packageManager field.

lukasholzer · 2022-10-25T11:26:10Z

We have enough information to detect the clash between the environment variable and the packageManager field. We could either

Let the user know there's a clash and fail the build

Let the user know there's a clash and that we are defaulting to the what is specified in the packageManager field.

Would love to get @JGAntunes ideas here as we cannot log here to the user (currently) The log output is used to print just the json out.

We have multiple options:

Option Add a --outfile=your.json to the command and redirect the stdout to the user. Additionally provide a --fileDescriptor flag which can be used to log debugging and humio information that is not visible to the customer.
Option. Do the detection logic inside buildbot and not here (my preferred one):

like

if os.Getenv("NETLIFY_USE_PNPM") == "true" && info.PackageManager.Name != "pnpm" {
  b.ClientLog.Errorf("We've detected that you are setting the NETLIFY_USE_PNPM environment variable but you are not using pnpm instead you are using %s", info.PackageManager.Name)
}

Inside buildbot we can still control which package manager needs to be used

jobala · 2022-10-25T11:44:28Z

Do the detection logic inside buildbot and not here (my preferred one)

This is what I was thinking as well.

lukasholzer · 2022-10-25T12:17:53Z

@jobala But then this PR should be good to go?

JGAntunes · 2022-10-25T12:35:53Z

Would love to get @JGAntunes ideas here as we cannot log here to the user (currently) The log output is used to print just the json out.

We could also have this module return an array of detected package managers which could work based on precedence? (Similar to framework-info - https://github.com/netlify/framework-info)

Buildbot could then act based on that output (say, if we've detected you have packageManager set to something but you're forcing something else via env vars)

jobala · 2022-10-25T12:55:51Z

Created 2332 to track the issue ⬆️

JGAntunes

Left a comment/question. Not a blocker at all 👍

JGAntunes · 2022-10-25T12:47:44Z

packages/build-info/src/detect-package-manager.ts

+  }
+
+  // find the correct lock file the tree up
+  const lockFilePath = await findUp(Object.keys(lockFileMap), { cwd })


We have access to the repo's root. If provided shall we stop searching at that level?

https://github.com/sindresorhus/find-up#stopat

good point will adapt 👍

…relative (#4643) * feat!: add packageManger information + make jsWorkspace packagePaths relative * chore: add further tests * chore: fix typo * chore: updates * chore: latest discussions * chore: fix tests on windows and old node * chore: add stopAt parameter for look up * chore: clean up

lukasholzer requested review from kitop, danez and jobala October 19, 2022 15:32

lukasholzer force-pushed the feat/add-new-print-workspaces-command branch 3 times, most recently from 5b91787 to 8185a24 Compare October 20, 2022 10:56

lukasholzer changed the title ~~feat: add new print-workspaces command~~ feat: add package manager detection Oct 20, 2022

lukasholzer force-pushed the feat/add-new-print-workspaces-command branch 2 times, most recently from 9a340ae to b8112d9 Compare October 20, 2022 16:00

danez reviewed Oct 21, 2022

View reviewed changes

packages/headers-parser/package.json Outdated Show resolved Hide resolved

danez reviewed Oct 21, 2022

View reviewed changes

packages/build-info/src/build-info.ts Outdated Show resolved Hide resolved

lukasholzer marked this pull request as draft October 21, 2022 08:29

lukasholzer force-pushed the feat/add-new-print-workspaces-command branch from b8112d9 to 5f7629c Compare October 21, 2022 09:25

lukasholzer changed the title ~~feat: add package manager detection~~ feat!: add packageManger information + make jsWorkspace packagePaths relative Oct 21, 2022

lukasholzer force-pushed the feat/add-new-print-workspaces-command branch 2 times, most recently from 5f3f37c to b9befb8 Compare October 21, 2022 09:34

lukasholzer commented Oct 21, 2022

View reviewed changes

lukasholzer marked this pull request as ready for review October 21, 2022 09:38

lukasholzer requested review from danez and JGAntunes October 21, 2022 09:38

danez reviewed Oct 21, 2022

View reviewed changes

packages/build-info/src/bin.ts Show resolved Hide resolved

packages/build-info/src/detect-package-manager.ts Outdated Show resolved Hide resolved

packages/build-info/src/detect-package-manager.ts Show resolved Hide resolved

packages/build-info/tests/test-setup.ts Show resolved Hide resolved

lukasholzer mentioned this pull request Oct 24, 2022

Installation step doesn't detect PNPM in a monorepo #4648

Closed

lukasholzer force-pushed the feat/add-new-print-workspaces-command branch from b9befb8 to 558415b Compare October 24, 2022 12:43

feat!: add packageManger information + make jsWorkspace packagePaths …

f85cd7c

…relative

lukasholzer force-pushed the feat/add-new-print-workspaces-command branch from 558415b to f85cd7c Compare October 24, 2022 13:08

jobala previously approved these changes Oct 24, 2022

View reviewed changes

packages/build-info/src/detect-package-manager.ts Show resolved Hide resolved

jobala reviewed Oct 25, 2022

View reviewed changes

packages/build-info/src/workspaces.ts Show resolved Hide resolved

chore: add further tests

4d06a07

github-actions bot added the Adds or modifies js files label Oct 25, 2022

lukasholzer requested review from jobala and danez October 25, 2022 10:09

chore: fix typo

2a42feb

jobala approved these changes Oct 25, 2022

View reviewed changes

Lukas Holzer added 5 commits October 25, 2022 16:48

Merge branch 'main' into feat/add-new-print-workspaces-command

c4711a1

chore: updates

cbcdfa8

chore: latest discussions

75d9b1b

Merge branch 'main' into feat/add-new-print-workspaces-command

86416e2

chore: fix tests on windows and old node

a83079e

JGAntunes reviewed Oct 27, 2022

View reviewed changes

Lukas Holzer added 2 commits October 27, 2022 13:01

chore: add stopAt parameter for look up

ae68800

chore: clean up

5194e86

lukasholzer merged commit 1b4eed8 into main Oct 27, 2022

lukasholzer deleted the feat/add-new-print-workspaces-command branch October 27, 2022 13:18

netlify-jenkins mentioned this pull request Oct 27, 2022

chore: release main #4658

Merged

netlify-jenkins mentioned this pull request Nov 30, 2022

chore: release main #4735

Merged

netlify-circleci mentioned this pull request Jul 26, 2023

chore: release main #5208

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: add packageManger information + make jsWorkspace packagePaths relative #4643

feat!: add packageManger information + make jsWorkspace packagePaths relative #4643

lukasholzer commented Oct 19, 2022 •

edited

Loading

lukasholzer Oct 21, 2022

lukasholzer Oct 21, 2022

lukasholzer Oct 21, 2022

lukasholzer Oct 21, 2022

jobala commented Oct 25, 2022

lukasholzer commented Oct 25, 2022

lukasholzer commented Oct 25, 2022

github-actions bot commented Oct 25, 2022

jobala commented Oct 25, 2022

lukasholzer commented Oct 25, 2022

jobala commented Oct 25, 2022 •

edited

Loading

lukasholzer commented Oct 25, 2022 •

edited

Loading

JGAntunes commented Oct 25, 2022

jobala commented Oct 25, 2022

JGAntunes left a comment

JGAntunes Oct 25, 2022

lukasholzer Oct 27, 2022

		import { exit, argv } from 'process'

		import yargs, { Arguments } from 'yargs'

		@@ -0,0 +1 @@
		export { getBuildInfo } from './get-build-info.js'

feat!: add packageManger information + make jsWorkspace packagePaths relative #4643

feat!: add packageManger information + make jsWorkspace packagePaths relative #4643

Conversation

lukasholzer commented Oct 19, 2022 • edited Loading

Summary

lukasholzer Oct 21, 2022

Choose a reason for hiding this comment

lukasholzer Oct 21, 2022

Choose a reason for hiding this comment

lukasholzer Oct 21, 2022

Choose a reason for hiding this comment

lukasholzer Oct 21, 2022

Choose a reason for hiding this comment

jobala commented Oct 25, 2022

lukasholzer commented Oct 25, 2022

lukasholzer commented Oct 25, 2022

github-actions bot commented Oct 25, 2022

jobala commented Oct 25, 2022

lukasholzer commented Oct 25, 2022

jobala commented Oct 25, 2022 • edited Loading

lukasholzer commented Oct 25, 2022 • edited Loading

JGAntunes commented Oct 25, 2022

jobala commented Oct 25, 2022

JGAntunes left a comment

Choose a reason for hiding this comment

JGAntunes Oct 25, 2022

Choose a reason for hiding this comment

lukasholzer Oct 27, 2022

Choose a reason for hiding this comment

lukasholzer commented Oct 19, 2022 •

edited

Loading

jobala commented Oct 25, 2022 •

edited

Loading

lukasholzer commented Oct 25, 2022 •

edited

Loading