Skip to content
This repository was archived by the owner on May 2, 2022. It is now read-only.

Add cross-env package and update package.json Node env scripts#149

Merged
joesnellpdx merged 1 commit into
10up:masterfrom
JustinyAhin:master
Nov 11, 2020
Merged

Add cross-env package and update package.json Node env scripts#149
joesnellpdx merged 1 commit into
10up:masterfrom
JustinyAhin:master

Conversation

@JustinyAhin
Copy link
Copy Markdown
Contributor

@JustinyAhin JustinyAhin commented Jul 2, 2019

Description of the Change

NODE_ENV is not a recognized command for Windows terminal and for older versions of PowerShell. That means that when running npm run dev, npm run build, etc on Windows machine, the script of package.json is not going to work.

So, I've added the package cross-env and change the NODE_ENV lines in the script to cross-env NODE_ENV so that it can be executed on any type of environnement.

Alternate Designs

Benefits

Execute package.json no matter the environment used.

Possible Drawbacks

The change doesn't seems to have drawbacks

Verification Process

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Applicable Issues

#148

@timwright12
Copy link
Copy Markdown
Contributor

@magnificode can you check this one out and confirm since you have a windows machine?

@timwright12 timwright12 requested a review from magnificode July 2, 2019 20:46
@samikeijonen
Copy link
Copy Markdown
Contributor

We have noticed need for cross-env in our personal project, +1 from me.

@samikeijonen
Copy link
Copy Markdown
Contributor

@magnificode Do you happen to have time to check this?

@magnificode
Copy link
Copy Markdown
Contributor

So so so sorry for the delay. This looks great, CSS and JS compile perfectly on Windows. 👍

@magnificode
Copy link
Copy Markdown
Contributor

@samikeijonen @timwright12 - for the record I'm compiling via WSL. The Linux subsystem on Windows doesn't require me to utilize PowerShell to compile which is what this PR solves.

In any case, if this solves the issue @JustinyAhin brought up, I'm all for it, as it will ensure that this theme is accessible to folks developing on machines with older versions of PowerShell!

Copy link
Copy Markdown
Contributor

@timwright12 timwright12 left a comment

Choose a reason for hiding this comment

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

We'll need to get this into the plugin repo before a merge

@samikeijonen
Copy link
Copy Markdown
Contributor

@samikeijonen
Copy link
Copy Markdown
Contributor

Any progress on this?

@ivankruchkoff
Copy link
Copy Markdown

Any updates here?

@jeffpaul
Copy link
Copy Markdown
Member

@timwright12 we good to resolve the merge conflict here and then merge this PR and the related one over in the plugin repo?

@samikeijonen
Copy link
Copy Markdown
Contributor

@joesnellpdx Can we get this in?

@smy315
Copy link
Copy Markdown
Contributor

smy315 commented Nov 10, 2020

+1 to get this merged. I'm not a Windows user myself but we had to add this exact update yesterday on a project for other Window users on the project.

@samikeijonen also noticed that there is a merge conflict now with this PR. Might want to update it so this can get merged in.

@joesnellpdx joesnellpdx merged commit da1962f into 10up:master Nov 11, 2020
@joesnellpdx
Copy link
Copy Markdown
Contributor

@samikeijonen @smy315 this is merged

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants