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: allow overriding usage of npx #2848

Open
wants to merge 1 commit into
base: next
Choose a base branch
from

Conversation

mnahkies
Copy link
Contributor

Description

I don't love this solution, but throwing it out there for discussion.

Problem

When workspaces are in play, npx will execute the command with the current working directory set to the workspace directory.

/root
  package.json
 packages
   my-evidence-proj <- CWD of the vite command ran by npx (incorrect)
     package.json
     pages
     evidence.plugins.yaml
     ...
     .evidence
       template <- CWD of the npx command (correct)
       tailwind.config.cjs

Which ultimately means that vite gets run in the wrong cwd and thus can't find the template / config.

I think arguably this is surprising behavior from npm / npx, but changing it would probably be a hard sell. Perhaps adding a flag to specify the cwd might be accepted?

Aside from this, personally I find the use of npx for this usecase to be a bit surprising - it's not clear to me how the dependency versions are being controlled.

Possible Solutions

  • Change npm to allow specifying the cwd when running npx / npm exec in a project using workspaces
  • Install all dependencies in project and run using yarn vite instead of npx
  • Explicitly pass the template root directory to vite
    • this almost works, but svelte-kit overrides it / ignores it and ends up being unable to find anything
    • if it's possible to explicitly pass the template root to svelte-kit instead of relying on the process.cwd() that would probably be the cleanest way to solve this

I've seen everything work with the use yarn to run vite option, and these additional dependencies installed (which also solves all the missing peerDependency warnings I get from the sample project):

    "@evidence-dev/trino": "^1.0.8",
    "@sveltejs/kit": "2.5.4",
    "@sveltejs/vite-plugin-svelte": "^3.0.0",
    "autoprefixer": "^10.4.20",
    "debounce": "^1.2.1",
    "git-remote-origin-url": "^4.0.0",
    "postcss": "^8.4.49",
    "postcss-load-config": "^4.0.1",
    "svelte": "4.2.19",
    "svelte-preprocess": "5.1.3",
    "svelte2tsx": "0.7.4",
    "tailwindcss": "^3.4.15",
    "unist-util-visit": "4.1.2",
    "vite": "^5.4.11"

Checklist

  • For UI or styling changes, I have added a screenshot or gif showing before & after
  • I have added a changeset
  • I have added to the docs where applicable
  • I have added to the VS Code extension where applicable

I don't love this solution, but throwing it out there for discussion.

*Problem*
When `workspaces` are in play, `npx` will execute the command with
the current working directory set to the `workspace` directory.

```
/root
  package.json
 packages
   my-evidence-proj <- CWD of the vite command ran by npx (incorrect)
     package.json
     pages
     evidence.plugins.yaml
     ...
     .evidence
       template <- CWD of the npx command (correct)
       tailwind.config.cjs
```

- https://github.com/npm/cli/blob/latest/lib/npm.js#L233-L248
- https://github.com/npm/cli/blob/latest/lib/commands/exec.js#L34-L38
- https://github.com/npm/cli/blob/latest/lib/commands/exec.js#L103

Which ultimately means that `vite` gets run in the wrong `cwd` and thus
can't find the template / config.

I think arguably this is surprising behavior from `npm` / `npx`, but
changing it would probably be a hard sell. Perhaps adding a flag to
specify the `cwd` might be accepted?

Aside from this, personally I find the use of `npx` for this usecase to
be a bit surprising - it's not clear to me how the dependency versions
are being controlled.

*Possible Solutions*
- Change `npm` to allow specifying the `cwd` when running `npx` / `npm exec`
  in a project using workspaces
- Install all dependencies in project and run using `yarn vite` instead
  of `npx`
- Explicitly pass the template root directory to `vite`
  - this almost works, but `svelte-kit` overrides it / ignores it and
    ends up being unable to find anything
  - if it's possible to explicitly pass the template root to svelte-kit
    instead of relying on the `process.cwd()` that would probably be the
    cleanest way to solve this
@mnahkies mnahkies had a problem deploying to Approval required to run action on external PR November 23, 2024 11:35 — with GitHub Actions Failure
@mnahkies mnahkies had a problem deploying to Approval required to run action on external PR November 23, 2024 11:35 — with GitHub Actions Failure
@@ -349,12 +363,18 @@ prog
prog
.command('preview')
.describe('preview the production build')
.option('--debug', 'Enables verbose console logs')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: pretty sure the omission of the --debug option here is a bug, it's referenced in the action

@ItsMeBrianD
Copy link
Member

ItsMeBrianD commented Nov 25, 2024

Great writeup and idea here - I like it, npm always loves to throw curveballs like this.

It would probably best to include the configuration in the new evidence.config.yaml file rather than providing it as a command line argument, I'm working with the assumption that a project will typically always use the same executable, and putting it in the configuration removes an opportunity for user error.

The configuration zod schema is here, and it can be accessed with the getEvidenceConfig function. See the template's svelte.config.js for an example usage.

I'm not sure what the best property to store this under would be - it could just live as executable at the base level, but I'm open to other ideas. It should default to npx and not being required in the configuration file.

Eventually we'll also be moving over to the new CLI which resolves this issue by directly using vite's API rather than creating a child process

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