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

Use npx for calling JS apps #1906

Closed
wants to merge 1 commit into from
Closed

Use npx for calling JS apps #1906

wants to merge 1 commit into from

Conversation

ChrisWiegman
Copy link
Member

Tasks

  • I have signed a Contributor License Agreement (CLA) with WP Engine.
  • If a code change, I have written testing instructions that the whole team & outside contributors can understand.
  • I have written and included a comprehensive changeset to properly document the changes I've made.

Description

With our mono repo we're hitting numerous issues in calling scripts such as the faust cli in various workspaces when work on the project itself (see https://github.com/wpengine/faustjs/actions/runs/9404217613/job/25902618771 for an example). This causes lost time and frustration and it isn't necessary.

To combat this, this PR replaces direct calls to apps such as faust, next and more by using npx to call the apps. This ensures they can be found in the mono repo, changes nothing for existing scripts that already work and reduces setup time and confusion when cloning the repo fresh or working with various packages such as the examples.

Testing

Testing can be accomplished with existing CI tools

Copy link

changeset-bot bot commented Jun 7, 2024

⚠️ No Changeset found

Latest commit: 03a071f

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

github-actions bot commented Jun 7, 2024

📦 Next.js Bundle Analysis for @faustwp/getting-started-example

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

@ChrisWiegman ChrisWiegman marked this pull request as ready for review June 7, 2024 14:07
@ChrisWiegman ChrisWiegman requested a review from a team as a code owner June 7, 2024 14:07
@theodesp
Copy link
Member

theodesp commented Jun 13, 2024

@ChrisWiegman I took a look at this. It looks like it's related to this issue:
npm/cli#4591

However I think the solution is simpler. We just have to run npm i after we use npm run build to install the faust cli binary to the project after it was build.

Here is the workflow that I used:

% git clone git@github.com:wpengine/faustjs.git faust2
Cloning into 'faust2'...
remote: Enumerating objects: 16369, done.
remote: Counting objects: 100% (1440/1440), done.
remote: Compressing objects: 100% (746/746), done.
remote: Total 16369 (delta 818), reused 1088 (delta 592), pack-reused 14929
Receiving objects: 100% (16369/16369), 41.77 MiB | 11.55 MiB/s, done.
Resolving deltas: 100% (9250/9250), done.
7s theo.despoudis ~/workspace
% cd faust2 
theo.despoudis ~/workspace/faust2
% npm i                         
% npm run dev -w examples/next/faustwp-getting-started                                                                                                                                                                           (canary)

> @faustwp/getting-started-example@0.1.0 dev
> faust dev

sh: faust: command not found
npm ERR! Lifecycle script `dev` failed with error: 
npm ERR! Error: command failed 
npm ERR!   in workspace: @faustwp/getting-started-example@0.1.0 
npm ERR!   at location: /Users/theo.despoudis/workspace/faust2/examples/next/faustwp-getting-started

% npm i                                                                                                                                                                  (canary)
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: '@faustwp/block-support-example@0.2.0',
npm WARN EBADENGINE   required: { node: '>=18', npm: '>=10' },
npm WARN EBADENGINE   current: { node: 'v18.17.0', npm: '9.6.7' }
npm WARN EBADENGINE }
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: '@faustwp/block-support-example@0.2.0',
npm WARN EBADENGINE   required: { node: '>=18', npm: '>=10' },
npm WARN EBADENGINE   current: { node: 'v18.17.0', npm: '9.6.7' }
npm WARN EBADENGINE }

changed 1 package, and audited 2861 packages in 2s

311 packages are looking for funding
  run `npm fund` for details

2 vulnerabilities (1 low, 1 high)

To address issues that do not require attention, run:
  npm audit fix

To address all issues (including breaking changes), run:
  npm audit fix --force

Run `npm audit` for details.


% npm run dev -w examples/next/block-support                                                                                                                             (canary)

> @faustwp/block-support-example@0.2.0 predev
> faust generatePossibleTypes && faust generateGlobalStylesheet

error - Could not find NEXT_PUBLIC_WORDPRESS_URL environment variable.
npm ERR! Lifecycle script `dev` failed with error: 
npm ERR! Error: command failed 
npm ERR!   in workspace: @faustwp/block-support-example@0.2.0 
npm ERR!   at location: /Users/theo.despoudis/workspace/faust2/examples/next/block-support 

We probably need to just update the DEVELOPMENT.md file to include this step.

@ChrisWiegman
Copy link
Member Author

After chatting with @theodesp on this, I'm going to close it and re-open with a documentation approach.

@ChrisWiegman ChrisWiegman deleted the use-npx branch June 14, 2024 12:15
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.

None yet

2 participants