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

build: directly generate js libs to node_modules #130

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

Conversation

chadoh
Copy link
Contributor

@chadoh chadoh commented Jul 20, 2023

What

Roll back #122, but keep crowdfund-contract and abundance-token in optionalDependencies.

This also adds a .gitattributes to suppress package-lock.json diffs.

Why

As I worked with the old version in earnest, iterating on a local copy of the CLI and generating new JS libs over and over, the two-step generate-to-.soroban, then install-to-node_modules wasn't working very well. Probably because the new library wasn't being added until the postinstall step, and the dependencies are optional.

Generating directly to node_modules feels dirty, but it works consistently. At least in this version, compared to the one before #122, you can still see the dependencies listed in the optionalDependencies section, so they're not quite as surprising/mysterious.

@chadoh
Copy link
Contributor Author

chadoh commented Jul 20, 2023

@tyvdh how's this look to you?

Copy link
Contributor

@kalepail kalepail left a comment

Choose a reason for hiding this comment

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

I don't love, but approve, of these changes.

At some point we need a Real NPM Pro™ to assess our jankyness and recommend the best case here. But for now, this is better than what we had and if it works better than what we have I'm all for it.

@chadoh chadoh force-pushed the build/better-gen branch 3 times, most recently from c9bb315 to 677122f Compare July 21, 2023 16:23
@chadoh
Copy link
Contributor Author

chadoh commented Jul 21, 2023

npm ci breaking in ci but not locally (╯°□°)╯︵ ┻━┻

@chadoh chadoh force-pushed the build/better-gen branch from 55b558e to d825f43 Compare August 8, 2023 15:42
@chadoh
Copy link
Contributor Author

chadoh commented Aug 8, 2023

@tyvdh the build finally passes 😅

Roll back stellar#122, but
keep `crowdfund-contract` and `abundance-token` in
`optionalDependencies`.

As I worked with the old version in earnest, iterating on a local copy
of the CLI and generating new JS libs over and over, the two-step
generate-to-.soroban, then install-to-node_modules wasn't working very
well. Probably because the new library wasn't being added until the
`postinstall` step, and the dependencies are optional.

Generating directly to `node_modules` feels dirty, but it works
consistently. At least in this version, you can still see the
dependencies listed in the `optionalDependencies` section, so they're
not quite as surprising/mysterious.
@kalepail
Copy link
Contributor

kalepail commented Aug 8, 2023

Looks like the deps are once again completely absent from package.json?

@chadoh chadoh force-pushed the build/better-gen branch from d825f43 to b0bebf6 Compare August 8, 2023 16:01
@chadoh
Copy link
Contributor Author

chadoh commented Aug 8, 2023

@tyvdh yes I was just looking into that; looks like they got removed from main here #127

I just added a commit that tries adding them back. Getting npm ci to actually pass on CI was quite difficult; we'll see if it works 🤞🏼

@kalepail
Copy link
Contributor

kalepail commented Aug 8, 2023

I just find it hard to believe there isn't a way to handle local packages. How are actual npm packages handling this with CI/CD flows?

@kalepail
Copy link
Contributor

kalepail commented Aug 8, 2023

Maybe helpful?
nodejs/node#46542 (comment)

@chadoh
Copy link
Contributor Author

chadoh commented Aug 8, 2023

Good find; trying it out with the latest commit

@chadoh chadoh force-pushed the build/better-gen branch 2 times, most recently from 9dd26ee to b9c3415 Compare August 8, 2023 19:52
@chadoh chadoh force-pushed the build/better-gen branch from b9c3415 to ccbae90 Compare August 8, 2023 19:52
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