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

Seungheonoh/npm submodule #1460

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

Conversation

SeungheonOh
Copy link

@SeungheonOh SeungheonOh commented Mar 20, 2023

Closes #1457.

Pre-review checklist

  • All code has been formatted using our config (make format)
  • Any new API features or modification of existing behavior is covered as defined in our test plan
  • The changelog has been updated under the ## Unreleased header, using the appropriate sub-headings (### Added, ### Removed, ### Fixed), and the links to the appropriate issues/PRs have been included

@SeungheonOh
Copy link
Author

SeungheonOh commented Mar 20, 2023

@aciceri @emiflake cc

nix/default.nix Outdated Show resolved Hide resolved
@klntsky klntsky self-requested a review March 23, 2023 15:55
Copy link
Member

@klntsky klntsky left a comment

Choose a reason for hiding this comment

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

See comment

@SeungheonOh
Copy link
Author

SeungheonOh commented Mar 23, 2023

Fixed, package.json and package-lock.json can't use farmLink because how node2nix uses it.

nix/default.nix Outdated Show resolved Hide resolved
Copy link
Member

@klntsky klntsky left a comment

Choose a reason for hiding this comment

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

I am not sure this is the right usage of linkFarm. cp -r is still used.

@SeungheonOh
Copy link
Author

SeungheonOh commented Mar 28, 2023

package.json and package-lock.json are expected to be a file to be used in node2nix. Not much can be done here.

For other part, I'm not sure if it can be done without it.

@klntsky klntsky requested a review from aciceri March 28, 2023 18:35
@SeungheonOh
Copy link
Author

@aciceri bump

Copy link
Collaborator

@aciceri aciceri left a comment

Choose a reason for hiding this comment

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

@SeungheonOh Only two perplexities:

  • what happens if submodules is []? I guess you do a cp <linkFarmOut>/* . -r where <linkFarmOut> is an empty folder. If you want you could directly avoid the entire line if there are no submodules
  • what happens if one wants to use a submodule which is the output of another derivation? For instance consider
submodules = [
  (pkgs.runCommand "foo-submodule" {} "mkdir -p $out/foo; magic > $out/foo")
]

The builtins.baseNameOf this derivation would be foo-submodule (I believe, haven't tested it). Is this what you want? I would make a test to check if this works.
Moreover I really don't like depending on builtins.baseNameOf, it looks flaky. But I can't give you any alternative that doesn't imply making the UI more complex:

submodules = [
 {
   name = "foo";
   path = ./foo-submodule;   
 }
 {
   name = "bar";
   path = mkBarSubmodule { inherit pkgs; src = ./bar-stuff; };   
 }
]

Said this, if it works it looks good to me, these are improvements that can be made when (and if) these problems arise.

@SeungheonOh
Copy link
Author

Thanks for the review. I haven't consider using output of derivation as a submodule. Mainly, it was intended for subdirectories.

Because I was mainly considering subdirectories, builtins.baseNameOf was good as it gave concise and consistent paths to be imported in package.json. That being said, I'm also happy with explicit path.

@klntsky
Copy link
Member

klntsky commented Apr 6, 2023

@SeungheonOh please implement explicit paths and avoid calling cp when submodules is [].

nix/default.nix Outdated Show resolved Hide resolved
Co-authored-by: emiflake <[email protected]>
@emiflake
Copy link
Contributor

emiflake commented Jun 1, 2023

Can this be merged? Or at least someone pick this up? We're still depending on this branch in several projects. cc: @SeungheonOh, @aciceri

@aciceri
Copy link
Collaborator

aciceri commented Jun 1, 2023

@klntsky In my opinion it's already mergeable.

@emiflake
Copy link
Contributor

emiflake commented Jul 3, 2023

Once again bumping this PR :)
@klntsky
If it needs a rebase, or whatnot I'd be happy to help.

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.

Nix does not support nested NPM module.
4 participants