Skip to content

make start:preview script cross-platform by replacing ';' with '&&'#1354

Open
dorothy122 wants to merge 2 commits into
cuttle-cards:mainfrom
dorothy122:main
Open

make start:preview script cross-platform by replacing ';' with '&&'#1354
dorothy122 wants to merge 2 commits into
cuttle-cards:mainfrom
dorothy122:main

Conversation

@dorothy122
Copy link
Copy Markdown
Contributor

Issue number

Relevant issue number

  • Resolves #

Please check the following

  • Do the tests still pass? (see Run the Tests)
  • Is the code formatted properly? (see Linting (Formatting))
  • For New Features:
    • Have tests been added to cover any new features or fixes?
    • Has the documentation been updated accordingly?

Please describe additional details for testing this change

  • Fixes a cross-platform issue in the 'start:preview' npm script
  • It previously used ; - npm run build;npm run start:server but this failed on Windows. So I have changed the ; to && so that it should run on all platforms. I have tested this on Windows but not on any other platforms.

Copy link
Copy Markdown
Contributor

@itsalaidbacklife itsalaidbacklife left a comment

Choose a reason for hiding this comment

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

Looks good. Requesting to remove "node" from dependencies and then this is ready

Comment thread package.json
"lodash": "4.17.21",
"machinepack-passwords": "2.3.0",
"marked": "^16.4.1",
"node": "^24.15.0",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Node shouldn't be required as a dependency of the project. It's instead listed under "engines" below.

Suggested change
"node": "^24.15.0",

Comment thread package.json
"start:client": "vite --host",
"start:server": "node app.js",
"start:preview": "npm run build;npm run start:server",
"start:preview": "npm run build && npm run start:server",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Interesting catch. Do I take it you use cmd? I'd only used powershell when working with windows and that uses ; the same way bash does, so I'd never encountered this issue. It seems a noninvasive improvement to cross compatibility

@itsalaidbacklife
Copy link
Copy Markdown
Contributor

As a last change, since you've updated a dependency, we need to update package-lock.json as well, which contains the exact versions of the full dependency tree. To do this, after removing node from package.json, please run
npm install then commit and push the changes that makes to package-lock.json

@itsalaidbacklife
Copy link
Copy Markdown
Contributor

@dorothy122 the need to update package-lock.json looks to be what caused the tests to fail since npm ci errors when the lock file is out of date.

First, remove 'node' from dependencies, and then you can either update the lock file with npm install and committing the changes that creates, or you can roll back the update to Vue devtools in package.json

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