Skip to content

Basic setup for building JS user library with Nix#61

Merged
mergify[bot] merged 4 commits intomasterfrom
paulyoung/js-user-library-prs
Oct 9, 2019
Merged

Basic setup for building JS user library with Nix#61
mergify[bot] merged 4 commits intomasterfrom
paulyoung/js-user-library-prs

Conversation

@paulyoung
Copy link
Contributor

Extracted from #56

@paulyoung paulyoung requested a review from a team October 8, 2019 23:48
Copy link
Contributor

@eftychis eftychis left a comment

Choose a reason for hiding this comment

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

LGTM -- not familiar with napalm -- seems straightforward.

@@ -0,0 +1,5 @@
= js-user-library
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this into a userlib/ and then separate by language?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would like to do this later if necessary since it will make the task of breaking up #56 even more complicated

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want us to rubberstamp every PR with my comments going to "later"? That's as if the PR was the 21k lines one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not at all, but this seems like an arbitrary decision that can wait until later.

@@ -1,2 +1,14 @@
{
"napalm": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this as part of our repo, or can we add it to common?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's specifically for building Node.js packages so I think it makes the most sense here for now. I can move it to common if other repos start using it.

self: super: {
dfinity-sdk = rec {
packages = rec {
js-user-library = super.callPackage ../../js-user-library/package.nix {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather have user-library-js or even better userlib.js.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would prefer to follow up with this later if necessary as well

@hansl
Copy link
Contributor

hansl commented Oct 9, 2019

I'll let @eftychis do the reviews of these, as I will be away for the next three weeks and he'll have to get familiar with the code (as well as work with you on it).

@mergify mergify bot merged commit 1bc28c1 into master Oct 9, 2019
@mergify mergify bot removed the automerge-squash label Oct 9, 2019
@paulyoung paulyoung deleted the paulyoung/js-user-library-prs branch October 9, 2019 23:24
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.

3 participants