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

build: Lock NPM dependencies for npm install #1286

Merged

Conversation

rzr
Copy link
Contributor

@rzr rzr commented Aug 22, 2018

I observed a regression caused to dependencies updates,
build is file but you can't use UI to add things
(browser complains on module.export = App)
Probably caused by babel update from beta49 to rc2 (TBC).

Project prefers yarn over npm,
but npm install could be used by developers or scripts.

Note that, Adding a (potentially unaligned) lock file,
could create ambiguity
but IMHO it's better to support both than only yarn.

Related links:

https://stackoverflow.com/questions/44552348/should-i-commit-yarn-lock-and-package-lock-json-files

yarnpkg/yarn#5654 (comment)

Change-Id: I9489e365b6d3a70102a7d2ae1e44feb7aeb28c06
Signed-off-by: Philippe Coval p.coval@samsung.com

I observed a regression caused to dependencies updates,
build is file but you can't use UI to add things
(browser complains on module.export = App)
Probably caused by babel update from beta49 to rc2 (TBC).

Project prefers yarn over npm,
but npm install could be used by developers or scripts.

Note that, Adding a (potentially unaligned) lock file,
could create ambiguity
but IMHO it's better to support both than only yarn.

Related links:

https://stackoverflow.com/questions/44552348/should-i-commit-yarn-lock-and-package-lock-json-files

yarnpkg/yarn#5654 (comment)

Change-Id: I9489e365b6d3a70102a7d2ae1e44feb7aeb28c06
Signed-off-by: Philippe Coval <p.coval@samsung.com>
@codecov-io
Copy link

codecov-io commented Aug 22, 2018

Codecov Report

Merging #1286 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1286   +/-   ##
=======================================
  Coverage   75.33%   75.33%           
=======================================
  Files         123      123           
  Lines        5916     5916           
  Branches      824      824           
=======================================
  Hits         4457     4457           
  Misses       1287     1287           
  Partials      172      172

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0ae5fab...11b1a2e. Read the comment docs.

@Swaagie
Copy link
Contributor

Swaagie commented Aug 22, 2018

Imho committing both lockfiles until npm and yarn settle on a solution would improve developer experience. Everybody benefits from having both. If you don't like big git diffs something we've been doing lately is adding a .gitattributes file with the following content:

package-lock.json binary

It will prevent git/github from showing the diffs. In addition the yarn.lock file could be added as well. If you still want to see the diffs (after updating a specific package) git diff --text will force show the diff.

@hobinjk
Copy link
Contributor

hobinjk commented Aug 22, 2018

Thanks for looking into this! I think in the future we want to switch to only NPM since yarn doesn't really have enough benefits to justify installing yet another binary on the pi. This is great step in that direction.

@hobinjk hobinjk merged commit 12648a1 into WebThingsIO:master Aug 22, 2018
@rzr
Copy link
Contributor Author

rzr commented Aug 22, 2018

Thanks, if anyone want to face the bug I described in commit message just rm lock files and install deps.

I am not yet familiar enough with babel to dig deeper

@Swaagie
Copy link
Contributor

Swaagie commented Aug 22, 2018

@rzr I can take a look at it tomorrow. I've spend way to much time in webpack and babel configs... 😛

@mrstegeman mrstegeman added this to Done in WebThings Gateway Apr 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants