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

Make package.json cross platform #8035

Merged
merged 5 commits into from
Aug 20, 2019
Merged

Make package.json cross platform #8035

merged 5 commits into from
Aug 20, 2019

Conversation

fzzylogic
Copy link
Contributor

CATEGORY

Choose one

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

SUMMARY

Make package.json cross platform (so besides Linux and Mac, it will work on Windows with cmd or powershell without modifications). Tested on Linux and Windows. Though webpack builds successfully on windows, the minified output is different.

Attempt at redemption after #8024 ^^

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

REVIEWERS

etr2460

Copy link
Member

@etr2460 etr2460 left a comment

Choose a reason for hiding this comment

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

since you're adding a new dependency, you need to run npm install and commit the lockfile changes as well

@kristw
Copy link
Contributor

kristw commented Aug 13, 2019

Also, what about other commands other than build?

@etr2460
Copy link
Member

etr2460 commented Aug 13, 2019

Also, what about other commands other than build?

It looks like build is the only command that sets an env variable, so i assume all the others work cross platform already

@codecov-io
Copy link

Codecov Report

Merging #8035 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8035      +/-   ##
==========================================
+ Coverage   65.54%   65.54%   +<.01%     
==========================================
  Files         469      469              
  Lines       22475    22487      +12     
  Branches     2440     2446       +6     
==========================================
+ Hits        14731    14739       +8     
- Misses       7624     7628       +4     
  Partials      120      120
Impacted Files Coverage Δ
superset/db_engine_specs/sqlite.py 45.83% <0%> (-20.84%) ⬇️
superset/utils/core.py 87.66% <0%> (-0.17%) ⬇️
superset/db_engine_specs/presto.py 77.98% <0%> (-0.09%) ⬇️
superset/viz.py 71.58% <0%> (-0.03%) ⬇️
superset/assets/src/chart/ChartRenderer.jsx 8.06% <0%> (ø) ⬆️
...src/SqlLab/utils/reduxStateToLocalStorageHelper.js 100% <0%> (ø) ⬆️
superset/config.py 88.7% <0%> (ø) ⬆️
superset/assets/src/SqlLab/reducers/sqlLab.js 48.38% <0%> (ø) ⬆️
...et/assets/src/dashboard/components/SliceHeader.jsx 24% <0%> (ø) ⬆️
superset/examples/deck.py 11.11% <0%> (ø) ⬆️
... and 6 more

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 5375745...497934f. Read the comment docs.

@fzzylogic
Copy link
Contributor Author

Introduced "response": "" instead of "response": false in package-lock in some cases accidentally, resolved with last commit. @etr2460 thanks for the guidance, @kristw yes, cross-env only solves the env var problem, however i didn't test all the other commands on Windows. Thanks!

Copy link
Member

@etr2460 etr2460 left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for this!

@kristw kristw merged commit c97a71b into apache:master Aug 20, 2019
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.35.0 labels Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XS 🚢 0.35.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants