Remove ImmutableJS, refactor search selectors #48

tiffon · 2017-08-08T17:28:59Z

Removed ImmutableJS (T1077867)

Issues with how the library was utilized (UI responsiveness of the search page is sluggish when many spans in results #43)
Incorrect usage can result in perf issues and bugs
Not ubiquitous knowledge - barrier to entry for open source contributors

Refactor search selectors

Data transformation for the search page was simplified and performs better. Added flow types for the derived data. Will eventually remove get-last-xform-cacher.js, but the way the components are organized needs to be adjusted, first.

saminzadeh · 2017-08-08T17:34:14Z

src/components/SearchTracePage/index.js

@@ -18,29 +18,23 @@
 // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
 // THE SOFTWARE.

+import _values from 'lodash/values';


Is this a common way of naming single lodash functions? Historically, this is generally meant for private variables.

Are we opposed to bring in the whole lib because of size? If so, I suppose we could do this style as long as we are consistent throughout the application.

@saminzadeh I have mixed feelings about the name, myself, for the same reason. I went with _values because

values would be an odd name for it as it's a function not a collection of values

Come to think of it, though, _values has basically the same issue

_values is reminiscent of _.values

I noticed another project using the same approach, but I'm definitely open to alternatives... Maybe valuesFn? (or lodashValues? Long names are a bother, though.)

Yes, importing by function (also called cherry-picking, I think) yields smaller builds. Consistency in the project, in this respect, is a WIP.

Ok cool, im open to the lodash function style of _values as an alias for _.values. It would be sweet if we could use this babel plugin but then we'd have to eject. https://github.com/lodash/babel-plugin-lodash

coveralls · 2017-08-08T17:42:21Z

Coverage decreased (-0.04%) to 55.351% when pulling 5f042f0 on refactor-search-selectors into 40b2e0f on master.

saminzadeh · 2017-08-08T17:36:21Z

src/model/order-by.js

@@ -0,0 +1,5 @@
+export const MOST_RECENT = 'MOST_RECENT';


I think this file needs license info, npm run add-license

Thanks. Done.

saminzadeh · 2017-08-08T17:38:16Z

src/model/search.js

+  const serviceMap = {};
+
+  let i = 0;
+  for (; i < spans.length; i++) {


Not familiar with this style? Is there an advantage than doing for (let i = 0; i < spans.length; i++)

Personal preference. Changed to the general style.

saminzadeh · 2017-08-08T17:43:56Z

src/reducers/trace.js

+}
+
+function fetchTraceErred(state, { meta, payload }) {
+  const traces = Object.assign({}, state.traces, { [meta.id]: payload });


Spread operator instead of Object.assign ?

I find it easier to read. For comarison, the diff.

Additional discussion on it from the redux crew: docs

Misread your comment. Good catch, done.

coveralls · 2017-08-08T20:10:55Z

Coverage increased (+0.2%) to 55.577% when pulling f1f7343 on refactor-search-selectors into 40b2e0f on master.

* Update prettier and wrap at 110 instead of 80 * Remove ImmutableJS from reducers * Remove remaining ImmutableJS-oriented functionality * Refactor search page mapStateToProps * Add flow-types and tests for model/search * Use spread operator instead of Object.assign * Misc inline comments * misc PR feedback Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>

tiffon added 9 commits August 3, 2017 18:06

Update prettier and wrap at 110 instead of 80

81bd57c

Remove ImmutableJS from reducers

75a014c

Remove remaining ImmutableJS-oriented functionality

288d04a

Merge branch 'remove-immutablejs' into refactor-search-selectors

717b162

Refactor search page mapStateToProps

42c0cb9

Add flow-types and tests for model/search

32fcca0

Use spread operator instead of Object.assign

661359d

Misc inline comments

9a3a2f4

Merge branch 'master' into refactor-search-selectors

5f042f0

tiffon requested review from yurishkuro and saminzadeh August 8, 2017 17:28

saminzadeh reviewed Aug 8, 2017

View reviewed changes

saminzadeh approved these changes Aug 8, 2017

View reviewed changes

misc PR feedback

f1f7343

tiffon merged commit 0753092 into master Aug 8, 2017

yurishkuro deleted the refactor-search-selectors branch January 29, 2020 15:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove ImmutableJS, refactor search selectors #48

Remove ImmutableJS, refactor search selectors #48

tiffon commented Aug 8, 2017

saminzadeh Aug 8, 2017

tiffon Aug 8, 2017

saminzadeh Aug 8, 2017

coveralls commented Aug 8, 2017

saminzadeh Aug 8, 2017

tiffon Aug 8, 2017

saminzadeh Aug 8, 2017

tiffon Aug 8, 2017

saminzadeh Aug 8, 2017

tiffon Aug 8, 2017

tiffon Aug 8, 2017

coveralls commented Aug 8, 2017

Remove ImmutableJS, refactor search selectors #48

Remove ImmutableJS, refactor search selectors #48

Conversation

tiffon commented Aug 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 8, 2017

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

coveralls commented Aug 8, 2017