Skip to content
This repository has been archived by the owner on Jan 6, 2022. It is now read-only.

Fix/clean npm scripts #551

Merged
merged 9 commits into from Oct 2, 2018
Merged

Fix/clean npm scripts #551

merged 9 commits into from Oct 2, 2018

Conversation

dkastl
Copy link
Contributor

@dkastl dkastl commented Oct 1, 2018

I found the npm scripts a bit difficult to understand, not only from the point of view of the names, but also how they were organized.

I tried to use a scope to group them together, where the "short" command is usually the one, that contains them all.

I haven't done anything with pack and dist. I didn't figure out yet, why there are 6 script entries necessary for something, that probably will be run with CI tools. I also can't think of a case, where all operating systems are built with a single run. I would suggest to reduce those to just pack and dist for the current OS.

Copy link
Collaborator

@martinheidegger martinheidegger left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! Those changes definitely look usable. I added a few comments. Would be nice if you could address them.

app/components/table-row.js Show resolved Hide resolved
package.json Outdated
"pretest": "npm run lint",
"test": "npm run test:unit",
"start": "cross-env NODE_ENV=development npm-run-all -p watch start:dev",
"start:dev": "cross-env NODE_V=\"$(node -v)\" NODE_ENV=development electron .",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe name "start: electron"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I had the same idea first. Will change.

package.json Outdated
"test": "npm run test:unit",
"start": "cross-env NODE_ENV=development npm-run-all -p watch start:dev",
"start:dev": "cross-env NODE_V=\"$(node -v)\" NODE_ENV=development electron .",
"watch": "webpack --watch --mode=development",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe "start:watch"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought changing this will make some people angry ;-)
But I agree with start:watch.

package.json Show resolved Hide resolved
@dkastl
Copy link
Contributor Author

dkastl commented Oct 2, 2018

FYI, npm install is not run anymore automatically after npm run clean. I think it makes the documentation more consistent to just always have to run npm install.

@dkastl
Copy link
Contributor Author

dkastl commented Oct 2, 2018

@martinheidegger npm-run-all -p start:watch start:electron does run both processes in parallel, but if you start from scratch, it seems that it may happen, that you first had to wait for start:watch to finish, because otherwise webpack would not have finished. The second time it all works.

@martinheidegger
Copy link
Collaborator

Oh right! Yes, that is an issue. We would need a start:build script to be executed before npm-run-all

@dkastl
Copy link
Contributor Author

dkastl commented Oct 2, 2018

Oh right! Yes, that is an issue. We would need a start:build script to be executed before npm-run-all

Can we just use build:dev script, because this already exists and isn't really used. build:prod is used for packaging and distributions. Or better rename it to start:build?

@martinheidegger
Copy link
Collaborator

Right build:dev is fine.

@martinheidegger
Copy link
Collaborator

Can you rebase again (with the removed package-lock.json)?

@dkastl
Copy link
Contributor Author

dkastl commented Oct 2, 2018

@martinheidegger would you mind to do this, because I don't have this cool Git software, that you have ;-)

I'm pretty much done here, so if you want to make this pull request prettier in terms of commit messages, rebasing, squashing and all that stuff, that I needed to consult a manual, please go ahead!

@dkastl dkastl added this to To Do in React via automation Oct 2, 2018
@dkastl dkastl changed the title [WIP] Fix/clean npm scripts Fix/clean npm scripts Oct 2, 2018
@dkastl
Copy link
Contributor Author

dkastl commented Oct 2, 2018

Rebase looks good to me.

@martinheidegger martinheidegger merged commit e8012fe into react Oct 2, 2018
React automation moved this from To Do to Done Oct 2, 2018
@martinheidegger martinheidegger deleted the fix/clean-npm-scripts branch October 2, 2018 07:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
React
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants