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: enable framework detection on js workspace root #5853

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

Conversation

lukasholzer
Copy link
Contributor

@lukasholzer lukasholzer commented Sep 26, 2024

Fixes FRB-793

It is valid to have a monorepo (PNPM workspace) but still have a site at the root of it.

This enables the support of still having detection inside monorepo packages but additionally on the root as well.

Currently released an RC version that we can test out carefully in the CLI 7.15.0-rc.0


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)

Fixes FRB-793

It is valid to have a monorepo (PNPM workspace) but still
have a site on the root of it

This enables the support of still having detection inside monorepo packages
but additionally on the root as well
Copy link

sonarcloud bot commented Sep 30, 2024

for (const result of results) {
for (let i = 0, max = rootFrameworks.length; i < max; i++) {
if (isEqual(result.detected, rootFrameworks[i].detected)) {
rootFrameworkMutex.runExclusive(async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you briefly explain why mutex is needed here (and only here)?

Copy link
Contributor

Choose a reason for hiding this comment

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

also - shouldn't this exclusive run be wider? index of item to remove is figured before entering exclusive mode and if mutex is here at all, presumably multiple concurrent runs can execute this code, so index figured out before acquiring mutex might no longer be correct because since figured out i something else might have mutated things?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh that's a good point!

yea because we run this highly in parallel so mutating the array is dangerous in a concurrent highly async loop!

* @param callback Function to be run exclusively.
* @returns The return value of `callback`.
*/
async runExclusive<T>(callback: () => Promise<T>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

could the same be achieved with just p-limit using 1 as concurrency? It seems like p-limit is already indirect dependency of build-info:

npm list p-limit
[email protected] /Users/misiek/dev/netlify-build
└─┬ @netlify/[email protected] -> ./packages/build-info
  ├─┬ @bugsnag/[email protected]
  │ └─┬ [email protected]
  │   └─┬ [email protected]
  │     └─┬ [email protected]
  │       └─┬ [email protected]
  │         └── [email protected]
  ├─┬ @vitest/[email protected]
  │ └─┬ [email protected]
  │   └─┬ [email protected]
  │     └─┬ [email protected]
  │       └─┬ [email protected]
  │         └── [email protected]
  ├─┬ [email protected]
  │ └─┬ [email protected]
  │   └─┬ [email protected]
  │     └── [email protected] deduped
  └─┬ [email protected]
    └─┬ @vitest/[email protected]
      └── [email protected]

(no strong preference here, just checking)

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