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

Share package.json scripts across packages #239

Merged
merged 10 commits into from
Apr 12, 2023

Conversation

spalladino
Copy link
Collaborator

@spalladino spalladino commented Apr 11, 2023

Sets up a shared package.scripts.json at the project root, which gets injected into each individual package.json that extends from it (via an inherits directive) whenever yarn prepare is run. Ended up using a custom script over https://github.com/microsoft/package-inherit since it didn't work with our workspace setup, seems unmaintained, and it was easier to write manually.

Something funny that happened after setting this up: forcing all packages to have the same set of scripts revealed that circuits and bb.js didn't have a prepare step, so their entry in the build_manifest wasn't getting updated. However, their entries depend on non-packages (ie wasm build), which cannot be expressed from the package json. So I added a near-empty yarn-project-base/package.json, which does depend on the wasm build in the build manifest, and added devDeps to it from circuits and bb js. This ensures we have a transitive dependency to wasm builds from those packages.

Fixes #202

@spalladino spalladino requested a review from ludamad April 11, 2023 18:28
@ludamad
Copy link
Collaborator

ludamad commented Apr 12, 2023

  1. run -T is pnp-speak I think. We should probably get rid of it
  2. Trying to figure out how I feel about this solution, feels a bit hacky to have code that gets overwritten (dependencies feel different as there is a single 'correct' configuration). Maybe a better paradigm is an ability to edit every relevant package at once?

@ludamad
Copy link
Collaborator

ludamad commented Apr 12, 2023

The script then would be able to copy scripts from a json to a set of packages given at command line. I guess it'd be nice if we had real inheritance, but this feels like a little too scrappy

@spalladino
Copy link
Collaborator Author

The script then would be able to copy scripts from a json to a set of packages given at command line.

What I'm worried about that solution is that it's easy for packages to get out of sync, for a dev to foget to include a set of packages, etc. After all, the issue with the missing prepare step was only uncovered by pushing the same config all over the place.

I agree that this feels scrappy, but manipulating the file directly is the only option given the lack of extensibility at the package.json level. Maybe adding a mechanism for individual packages to override a specific script would help? Or would it add to the scrappiness?

@ludamad
Copy link
Collaborator

ludamad commented Apr 12, 2023

I'm having trouble thinking of a better solution besides having some 'yarn common build' instead of yarn build, and routing everything through a common command. OK with this though for lack of a much better idea

@spalladino spalladino merged commit 31d463a into master Apr 12, 2023
@spalladino spalladino deleted the palla/package-json-inherit branch April 12, 2023 18:52
ludamad pushed a commit that referenced this pull request Apr 14, 2023
* fix bug in native kernel - contract logic was being skipped

* zero-initialize all abi and kernel structs. helper function to zero-init arrays of frs

* better struct initialization in app circuits files

* dont need to do as much in base utils since structs are sanely zero-initialized

* use fill to initialize array of fields to zero

* fix native private kernel, refactor private kernel tests, add real logic in test setup for computing function/contract tree roots/siblings

* new hash functions
ludamad pushed a commit that referenced this pull request Apr 17, 2023
* fix bug in native kernel - contract logic was being skipped

* zero-initialize all abi and kernel structs. helper function to zero-init arrays of frs

* better struct initialization in app circuits files

* dont need to do as much in base utils since structs are sanely zero-initialized

* use fill to initialize array of fields to zero

* fix native private kernel, refactor private kernel tests, add real logic in test setup for computing function/contract tree roots/siblings

* new hash functions
ludamad pushed a commit that referenced this pull request Apr 17, 2023
* fix bug in native kernel - contract logic was being skipped

* zero-initialize all abi and kernel structs. helper function to zero-init arrays of frs

* better struct initialization in app circuits files

* dont need to do as much in base utils since structs are sanely zero-initialized

* use fill to initialize array of fields to zero

* fix native private kernel, refactor private kernel tests, add real logic in test setup for computing function/contract tree roots/siblings

* new hash functions
ludamad pushed a commit that referenced this pull request Jul 14, 2023
codygunton pushed a commit that referenced this pull request Jan 23, 2024
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.

Evaluate package json inheritance to reduce duplicated scripts
2 participants