Update create-react-app to 2.1.2 #302

tiffon · 2019-01-02T23:03:29Z

Update create-react-app to 2.1.2 from 1.x.

Involved updating react-app-rewired: timarney/react-app-rewired#346.

More than a few files are changed due to prettier formatting. The first commit is the changes necessary to update to 2.1.2.

yurishkuro · 2019-01-02T23:05:07Z

could we not separate prettier formatting to a different PR?

yurishkuro · 2019-01-02T23:06:11Z

what is CRA 2 ?

tiffon · 2019-01-03T22:38:27Z

CRA is create-react-app, and CRA 2 is version >= 2.

We can separate the prettier formatting into a separate PR, I was thinking having separate commits is sufficient distinction.

yurishkuro · 2019-01-03T23:55:49Z

I was thinking having separate commits is sufficient distinction.

once merged it will be squashed into one commit, which will be difficult to review if needed later.

everett980

aside from question about ^ in package.json everything else is super minor and/or pre-existing. also CI error

everett980 · 2019-01-04T21:05:14Z

packages/jaeger-ui/package.json

-    "react-scripts": "^1.0.11",
+    "react-app-rewire-less": "2.1.3",
+    "react-app-rewired": "2.0.2-next.0",
+    "react-scripts": "2.1.2",


it seems as though nearly everything in the package.json that wasn't touched has a ^ (except antd, plexus (which may not matter because it uses the relative path?), and u-basscss). Is there a rule on when to use exact version?

Lately, I've been going for exact, but it's not a big deal as we're using a yarn.lock file. Seems more important when publishing npm packages.

everett980 · 2019-01-04T21:06:52Z

packages/jaeger-ui/package.json

@@ -96,5 +86,6 @@
      "!src/utils/test/**/*.js",
      "!src/demo/**/*.js"
    ]
-  }
+  },
+  "browserslist": [">0.2%", "not dead", "not ie <= 11", "not op_mini all"]


do we want to have such a permissive list? if so, should we have separate builds for more modern browsers so that the builds can leverage more recent browser changes?

I increased to 0.5%:

and_chr 70 and_uc 11.8 -android 4.4.3-4.4.4 -android 4.2-4.3 chrome 70 chrome 69 -chrome 68 -chrome 67 -chrome 63 chrome 49 edge 17 firefox 63 -firefox 52 ios_saf 12.0-12.1 ios_saf 11.3-11.4 ios_saf 11.0-11.2 -ios_saf 10.3 -ios_saf 10.0-10.2 -ios_saf 9.3 opera 56 safari 12 -safari 11.1 -safari 10.1 samsung 4

everett980 · 2019-01-04T21:48:27Z

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/ListView/index.js

-    this._startIndexDrawn = 2 ** 20;
-    this._endIndexDrawn = -(2 ** 20);
+    // eslint-disable-next-line no-bitwise
+    this._startIndexDrawn = 1 << 20;


is there a reason to use this specific number? or should we use Number.MAX_SAFE_INTEGER?

everett980 · 2019-01-04T21:57:53Z

packages/jaeger-ui/src/utils/number.test.js

  expect(numberUtils.toFloatPrecision(0.51, 1)).toBe(0.5);
  expect(numberUtils.toFloatPrecision(-0.307, 2)).toBe(-0.31);
+  // Had an issue with expect(-0).toBe(0) failing


could the expected value be updated then? expect(numberUtils.toFloatPrecision(-0.026, 1)).toBe(-0)?

everett980 · 2019-01-04T22:01:32Z

packages/plexus/demo/src/data-small.js

+        value: new Date(),
+        message: 'vertex w a string key that has spaces',
+      },
+    },
    {
      key: 1,
      label: 'Key is the number 1Key is the number 1Key',


pre-existing: is there a reason this key is this way? seems odd but nbd

Requires changes from PR #346 which are published as react-app-rewired@2.0.2-next.0 timarney/react-app-rewired#346 Signed-off-by: Joe Farro <joef@uber.com>

Signed-off-by: Joe Farro <joef@uber.com>

Make sure the ./packages/jaeger-ui uses the ./.eslintrc. Make sure all tests are run in pre-commit hook. CRA is now failing builds in CI if there are any webpack build warnings: https://github.com/facebook/create-react-app/blob/73e3d0ebf1f2834e1c8c41d3a25ae5e0e99e6f14/packages/react-scripts/scripts/build.js#L171-L184 Signed-off-by: Joe Farro <joef@uber.com>

Signed-off-by: Joe Farro <joef@uber.com>

codecov · 2019-01-06T18:20:24Z

Codecov Report

Merging #302 into master will decrease coverage by 0.06%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #302      +/-   ##
==========================================
- Coverage   82.52%   82.45%   -0.07%     
==========================================
  Files         141      140       -1     
  Lines        3153     3152       -1     
  Branches      654      651       -3     
==========================================
- Hits         2602     2599       -3     
- Misses        437      442       +5     
+ Partials      114      111       -3

Impacted Files	Coverage Δ
.../jaeger-ui/src/components/SearchTracePage/index.js	`89.28% <ø> (ø)`	⬆️
.../components/SearchTracePage/SearchResults/index.js	`75% <ø> (ø)`	⬆️
packages/jaeger-ui/src/model/trace-dag/TraceDag.js	`58.69% <ø> (ø)`	⬆️
...ckages/jaeger-ui/src/model/trace-dag/DenseTrace.js	`96.96% <ø> (ø)`	⬆️
...s/SearchTracePage/SearchResults/ResultItemTitle.js	`80% <ø> (ø)`	⬆️
packages/jaeger-ui/src/model/span.js	`100% <ø> (ø)`	⬆️
...onents/TracePage/TracePageHeader/AltViewOptions.js	`0% <ø> (ø)`	⬆️
packages/jaeger-ui/src/model/search.js	`100% <ø> (ø)`	⬆️
.../src/components/TracePage/TraceGraph/TraceGraph.js	`89.7% <ø> (ø)`	⬆️
...e/TraceTimelineViewer/SpanDetail/KeyValuesTable.js	`91.17% <100%> (+0.26%)`	⬆️
... and 5 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 f23eb48...9883067. Read the comment docs.

Signed-off-by: Joe Farro <joef@uber.com>

Avoid issue yarnpkg/yarn#6300 Signed-off-by: Joe Farro <joef@uber.com>

everett980

looks good to me

everett980 · 2019-01-07T16:49:47Z

packages/jaeger-ui/src/model/search.js

@@ -32,6 +32,7 @@ const comparators = {
 * @param  {Trace[]} traces The `Trace` array to sort.
 * @param  {string} sortBy A sort specification, see ./order-by.js.
 */
+// eslint-disable-next-line import/prefer-default-export


I saw this come up while working, maybe we should disable this rule? In order to make mocks and spies in tests easier, I found it beneficial to only default export react components. utils and what not do not need to be default exported.

I think it only comes up when there is only one export in a file.

Signed-off-by: Joe Farro <joef@uber.com>

* WIP upgrade to create-react-app v2.1.2 Requires changes from PR jaegertracing#346 which are published as react-app-rewired@2.0.2-next.0 timarney/react-app-rewired#346 Signed-off-by: Joe Farro <joef@uber.com> * Use node 8, less liberal browser support, fix test Signed-off-by: Joe Farro <joef@uber.com> * Use eslintrc, fix CI build, fix pre-commit hook Make sure the ./packages/jaeger-ui uses the ./.eslintrc. Make sure all tests are run in pre-commit hook. CRA is now failing builds in CI if there are any webpack build warnings: https://github.com/facebook/create-react-app/blob/73e3d0ebf1f2834e1c8c41d3a25ae5e0e99e6f14/packages/react-scripts/scripts/build.js#L171-L184 Signed-off-by: Joe Farro <joef@uber.com> * Skip react-vis.css format check, fail-fast in CI Signed-off-by: Joe Farro <joef@uber.com> * Don't collect coverage from dev proxy setup Signed-off-by: Joe Farro <joef@uber.com> * Upgrade react-app-rewired to 2.0.1 Avoid issue yarnpkg/yarn#6300 Signed-off-by: Joe Farro <joef@uber.com> * Cleanup npm packages in packages/jaeger-ui Signed-off-by: Joe Farro <joef@uber.com> Signed-off-by: Everett Ross <reverett@uber.com>

* WIP upgrade to create-react-app v2.1.2 Requires changes from PR jaegertracing#346 which are published as react-app-rewired@2.0.2-next.0 timarney/react-app-rewired#346 Signed-off-by: Joe Farro <joef@uber.com> * Use node 8, less liberal browser support, fix test Signed-off-by: Joe Farro <joef@uber.com> * Use eslintrc, fix CI build, fix pre-commit hook Make sure the ./packages/jaeger-ui uses the ./.eslintrc. Make sure all tests are run in pre-commit hook. CRA is now failing builds in CI if there are any webpack build warnings: https://github.com/facebook/create-react-app/blob/73e3d0ebf1f2834e1c8c41d3a25ae5e0e99e6f14/packages/react-scripts/scripts/build.js#L171-L184 Signed-off-by: Joe Farro <joef@uber.com> * Skip react-vis.css format check, fail-fast in CI Signed-off-by: Joe Farro <joef@uber.com> * Don't collect coverage from dev proxy setup Signed-off-by: Joe Farro <joef@uber.com> * Upgrade react-app-rewired to 2.0.1 Avoid issue yarnpkg/yarn#6300 Signed-off-by: Joe Farro <joef@uber.com> * Cleanup npm packages in packages/jaeger-ui Signed-off-by: Joe Farro <joef@uber.com> Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>

* WIP upgrade to create-react-app v2.1.2 Requires changes from PR jaegertracing#346 which are published as react-app-rewired@2.0.2-next.0 timarney/react-app-rewired#346 Signed-off-by: Joe Farro <joef@uber.com> * Use node 8, less liberal browser support, fix test Signed-off-by: Joe Farro <joef@uber.com> * Use eslintrc, fix CI build, fix pre-commit hook Make sure the ./packages/jaeger-ui uses the ./.eslintrc. Make sure all tests are run in pre-commit hook. CRA is now failing builds in CI if there are any webpack build warnings: https://github.com/facebook/create-react-app/blob/73e3d0ebf1f2834e1c8c41d3a25ae5e0e99e6f14/packages/react-scripts/scripts/build.js#L171-L184 Signed-off-by: Joe Farro <joef@uber.com> * Skip react-vis.css format check, fail-fast in CI Signed-off-by: Joe Farro <joef@uber.com> * Don't collect coverage from dev proxy setup Signed-off-by: Joe Farro <joef@uber.com> * Upgrade react-app-rewired to 2.0.1 Avoid issue yarnpkg/yarn#6300 Signed-off-by: Joe Farro <joef@uber.com> * Cleanup npm packages in packages/jaeger-ui Signed-off-by: Joe Farro <joef@uber.com> Signed-off-by: Everett Ross <reverett@uber.com> Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>

tiffon requested a review from everett980 January 2, 2019 23:03

ghost assigned tiffon Jan 2, 2019

ghost added the review label Jan 2, 2019

everett980 approved these changes Jan 4, 2019

View reviewed changes

WIP upgrade to create-react-app v2.1.2

db8f1c7

Requires changes from PR #346 which are published as react-app-rewired@2.0.2-next.0 timarney/react-app-rewired#346 Signed-off-by: Joe Farro <joef@uber.com>

tiffon force-pushed the update-create-react-app-2 branch from e2d8b4a to db8f1c7 Compare January 6, 2019 11:44

tiffon added 3 commits January 6, 2019 07:07

Use node 8, less liberal browser support, fix test

de7189f

Signed-off-by: Joe Farro <joef@uber.com>

Skip react-vis.css format check, fail-fast in CI

e516d15

Signed-off-by: Joe Farro <joef@uber.com>

tiffon added 2 commits January 6, 2019 13:29

Don't collect coverage from dev proxy setup

625883a

Signed-off-by: Joe Farro <joef@uber.com>

Upgrade react-app-rewired to 2.0.1

9ec38d5

Avoid issue yarnpkg/yarn#6300 Signed-off-by: Joe Farro <joef@uber.com>

everett980 approved these changes Jan 7, 2019

View reviewed changes

Cleanup npm packages in packages/jaeger-ui

9883067

Signed-off-by: Joe Farro <joef@uber.com>

tiffon merged commit 7a6190e into master Jan 9, 2019

ghost removed the review label Jan 9, 2019

yurishkuro deleted the update-create-react-app-2 branch January 29, 2020 15:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update create-react-app to 2.1.2 #302

Update create-react-app to 2.1.2 #302

tiffon commented Jan 2, 2019

yurishkuro commented Jan 2, 2019

yurishkuro commented Jan 2, 2019

tiffon commented Jan 3, 2019

yurishkuro commented Jan 3, 2019

everett980 left a comment •

edited

Loading

everett980 Jan 4, 2019

tiffon Jan 6, 2019

everett980 Jan 4, 2019

tiffon Jan 6, 2019

everett980 Jan 4, 2019

everett980 Jan 4, 2019

everett980 Jan 4, 2019

codecov bot commented Jan 6, 2019 •

edited

Loading

everett980 left a comment

everett980 Jan 7, 2019

tiffon Jan 9, 2019

Update create-react-app to 2.1.2 #302

Update create-react-app to 2.1.2 #302

Conversation

tiffon commented Jan 2, 2019

yurishkuro commented Jan 2, 2019

yurishkuro commented Jan 2, 2019

tiffon commented Jan 3, 2019

yurishkuro commented Jan 3, 2019

everett980 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 6, 2019 • edited Loading

Codecov Report

everett980 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

everett980 left a comment •

edited

Loading

codecov bot commented Jan 6, 2019 •

edited

Loading