feature: typescript support, build optimizations and currency support #125

dreadera · 2021-05-26T07:49:08Z

The Problem

First of all I Apologize for the large chunk of changes. I tried to minimize it as much as possible.
This entire library has been stuck in a bit of a time machine. It's quite dated with failing tests, mismatched versioning and lack of proper development tools.

The List of Problems:

Lack of TypeScript Support
Failing Builds
Failing Tests
Webpack
Chunky Bundle
No Currency Support for the Currency Type Filter

The Goal

Update dependencies.
Introduce new building tools.
TypeScript support.
Fix the buggy tests
Add Multiple Currency / Locale Number Formatting

What has been done

Introduced Rollup as our default bundler.
Introduced TypeScript support.
Introduced the useLocaleNumber hook.
Introduced new Currency Filter Type Component.
Introduced React Context Provider to act as a store.
Existing Test Cases Updated.
Existing Storybooks Updated.
New Tests cases Added.
Updated Storybooks.
Updated Version Number.
Dependency Clean up

Instead of Weback, We are now using Rollup as our bundler as it has better build times and better support for JavaScript/TypeScript Library building. Side Effects: - Structure is slightly different but importing is the same

Some of the functionality was failing for the most part. I have redid the snapshots using the -u flag. useLocaleNumber hook now provides formatted numbers based on the provided Locale and Currency.

…ats.

List.js - getVisibleColumns was being imported from the wrong place Table.js - Selection would be 'all' and it would not be filterable. instead i have removed the filter if selection is all

uptickmetachu · 2021-05-27T23:06:03Z

My slight concern is we are going from one ball of complexity to another (webpack -> rollup). There seems to be equally a lot of configuration required WRT babel/eslint/rollup as there was before.

I do appreciate the better build times and bundle size. I wonder if there is a way to simplify the build? Also have we tested this with yarn link against workforce to make sure the built version works with workforce (minus the janky stuff in workforce webpack).

uptickmetachu

Same as my comments above, otherwise looking good!

Regarding the actual frontend changes can you add a comment to sections that reviewers should look at? It has become hard to navigate due to all the format changes.

uptickmetachu · 2021-05-27T23:09:10Z

package.json

+    "style-loader": "^2.0.0",
+    "ts-jest": "^26.5.6",
+    "typescript": "^4.2.4",
+    "typescript-eslint": "^0.0.1-alpha.0"


o,o is this the correct plugin?
https://www.npmjs.com/package/@typescript-eslint/eslint-plugin
Perhaps we mean this one?

oops, you're right. I will rectify this :)

uptickmetachu · 2021-05-27T23:10:15Z

rollup.config.js

+  output: [
+    // Common JS Support.
+    {
+      file: packageJson.main,


oh neat! I like this importing the package.json and referencing the file for linking the build tools to the packaging. 👍🏼

uptickmetachu · 2021-05-27T23:11:21Z

rollup.config.js

+      },
+    }),
+
+    // Do Babel transpilation.


:( Somehow I assumed we could get rid of babel if we adopted TS.

I believe we still need to transpile it back to es6 - I don't think tsc does that, does it?

dreadera · 2021-05-27T23:42:15Z

I fully agree that going from webpack -> rollup doesn't change things in terms of configuration and does add a bit of complexity if you don't know rollup but i chose it simply for the sake of having a large amount of community maintained plugins that work out of the box without much configuration. while the actually rollup config itself does require you incorporate the plugins, the plugins themselves don't need much configuration.

if it makes things easier, we can try esbuilding everything without having rollup :) the configuration is also minimal for that :))

dreadera · 2021-05-27T23:45:26Z

In terms of testing with workforce, I haven't fully tried every place where this library is used but I did yarn link and have a go on a demo version I setup, which worked as intended :)

adamdickinson

Relatively quick pass from me. Awesome attention to detail in many parts - loving what you've done! I've noted a few things, queried a few parts and suggested some things along the way.

I will flag that I'm concerned we won't be able to give this a thorough and proper review due to its size, but I'll do my best :)

Also apologies for the short/sharp/inconsiderate comments! Loving your work, just short on time this morning 😬.

adamdickinson · 2021-05-27T23:17:04Z

package.json

 {
  "name": "react-object-list",
-  "version": "0.2.10",
+  "version": "0.3.00",


This could be a typo? Also, what is the breaking change with this new version?

Suggested change

"version": "0.3.00",

"version": "0.3.0",

I only changed the middle value to represent the minor update in accordance with SemVer to represent the new currency feature.

adamdickinson · 2021-05-27T23:18:16Z

src/ObjectList.demo.stories.js

 import List from './data-renderers/List'

-const mockData = require('./demo.data.json')
+const mockData = require('../samples/people.json')


Thought - could we lean on something like faker to generate this data for us?

We should. Faker is really nice for dev / showcasing :)

adamdickinson · 2021-05-27T23:20:23Z

.eslintrc

-    "babel"
-  ],
-  "parser": "babel-eslint",
+  "plugins": ["babel", "react", "react-hooks", "@typescript-eslint"],


Do you think it could be worth us creating an uptick/lint-config package to keep all our standard eslint (and prettier) rules in the one place? It'll make it easy for us to have the one, unified uptick standard across the board.

As a side-thought, this would be nice too - just want to drop the idea of it though:
https://github.com/tclindner/npm-package-json-lint

I was thinking the same thing. I think we should all have a standardised version which should be version controlled and then be available via npm :)

adamdickinson · 2021-05-27T23:24:48Z

rollup.config.js

+      },
+    }),
+
+    // Do Babel transpilation.


I believe we still need to transpile it back to es6 - I don't think tsc does that, does it?

adamdickinson · 2021-05-28T00:23:08Z

src/filters/types/__tests__/Date.test.js

 import React from 'react'
 import { shallow } from 'enzyme'
-import { snapshotTest } from 'utils/tests'
+import { snapshotTest } from '../../../../utils/tests'


Out of curiosity - what benefit do we get from switching away from the aliased paths?

adamdickinson · 2021-05-28T00:24:05Z

src/filters/types/__tests__/__snapshots__/currency.test.tsx.snap

@@ -0,0 +1,16 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`Currency Snapshots enders default 1`] = `


Typo here too?

adamdickinson · 2021-05-28T00:25:58Z

src/hooks/use-locale-number.test.tsx

+  return <div>{format(value)}</div>
+}
+
+describe('Convert Number To Locale', () => {


Question on consistency - we use lowercase in our other test files, but Title Case throughout this one. I think the convention is the former. Could we update to lowercase to stay with convention?

Suggested change

describe('Convert Number To Locale', () => {

describe('convert number to locale', () => {

adamdickinson · 2021-05-28T00:27:05Z

src/hooks/use-locale-number.ts

+
+    // add override from context if provided.
+    ...context,
+
+    // add override if params are provided.
+    ...params,


I think you've done a good enough job making the code explain this for you :)

Suggested change

// add override from context if provided.

...context,

// add override if params are provided.

...params,

...context,

...params,

src/hooks/use-locale-number.ts

adamdickinson · 2021-05-28T00:35:24Z

src/utils/translate.ts

+export default function translate(path: string) {
+  const travel = regexp =>
+    String.prototype.split
+      .call(path, regexp)
+      .filter(Boolean)
+      .reduce((res, key) => (res !== null && res !== undefined ? res[key] : res))
+
+  const result = travel(/[,[\]]+?/) || travel(/[,[\].]+?/)
+  const payload = result === undefined
+
+  return payload
+}


What... does this function do?

Ah, I forgot to remove this. I was planning to write a locale function to get strings from objects. like lodash/get. however, i moved away from this. I just forgot to remove the function.

Co-authored-by: Adam Dickinson <adam@renegadedigital.com.au>

jarekwg

Mammoth effort!

I'm not strong on reviewing frontend code, so just going to provide cursory comments.

The size of this PR is very hard to review properly. If it's relatively straightforward to cut a couple more branches from master, cherry pick individual changes into them, and open up a couple of smaller PRs, I'd strongly recommend that. This is especially important when it comes to codestyle changes vs business logic changes. At the very least, the currency changes need to be moved out of here and done separately.
My vote would be to use esbuild over rollup here too, just so that we're not spreading ourselves thing on bundler prerequisite knowledge..

Love what you've done, but yeah if we can get it through in some smaller logical smaller chunks, will be a much saner exercise for all involved.

jarekwg · 2021-05-28T01:26:38Z

src/filters/types/Currency.tsx

+Currency.defaultProps = {
+  currencySymbol: 'AUD',
+}


This doesn't look right -- "currencySymbol" suggests '$', not 'AUD'..
The way I'd see this working is the widget reads in locale info from a higher context, and then applies the appropriate formatting.

The native Intl.NumberFormat function takes in the standard currency symbols.

This works a lot better as it's built into JS and means we only have to provide the following. Locale, Currency Format and Decimals to the context provider upon invocation.

Function: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/NumberFormat/NumberFormat
List: https://www.currency-iso.org/en/home/tables/table-a1.html

gotcha; can we rename it just to currency then?

dreadera · 2021-05-28T02:25:03Z

I 100% agree that this was should have been broken down into multiple PRs. I do apologise for that.

jarekwg · 2021-05-28T04:53:10Z

Ok, if not planning to split this up, we'll want to publish this with at least a minor version release bump (ie 0.3.0). Then will probs be best to test by plugging it directly into workforce and clicking through to see how it all works. I suspect there'll need to be followup patch versions needed for the stuff we haven't picked up in this review.

scaredcat · 2021-06-01T07:00:20Z

The changes here won't quite work. This package is set up kind of like a PWA where

src/index is the skeleton app. Placeholders for the filers (top section) and renderer (table/list)
src/filters/index is the filters to display at the top of the app
src/data-renderers/index is the content of the app (also imported directly into the skeleton app -- bad practice, but programmers are lazy. 99% of the time we will use Table renderer)
src/icons -- us trying to be fancy with too much customization. We should just use one icon set and stick with that.
src/types aka cells -- fine grained control of the type of data rendered within a table

In your changes rollup.config.js only references src/index which also includes the default data renderer and probably some cells. Its tree-shaking functionality is quite good so it will get rid of things that are not exported/referenced.

Please either

Export all the things from src/index to make sure it comes with all the necessary parts (preferred option)
Update rollup to continue delivering this package as a Frankenstein for self-assembly at the other end

dreadera · 2021-06-01T11:25:55Z

wow, thanks for all that. it's nice to get some feedback on someone who worked on it :)
I'll improve this PR with the notes you have given. Thank you @scaredcat

dreadera added 12 commits May 24, 2021 07:00

Feature: New Build Method & Structure Cleanup.

4dd012f

Instead of Weback, We are now using Rollup as our bundler as it has better build times and better support for JavaScript/TypeScript Library building. Side Effects: - Structure is slightly different but importing is the same

Renaming JSX Files to JS.

69c8f05

Fixing Up Formatting

d67ed9f

Updated Snapshots from Jest & Added useLocaleNumber Hook.

c3c667b

Some of the functionality was failing for the most part. I have redid the snapshots using the -u flag. useLocaleNumber hook now provides formatted numbers based on the provided Locale and Currency.

Finished Writing The Tests For the Hook Configuration.

ae35c0e

Added Currency Component and It's tests with the useLocaleNumber Hook.

04d6ade

File Format Fix.

b015fb6

Export Adjustment.

274ebb8

Converted the Currency Filter Type To Functional and Added New Tests.

03125fa

New Currency Component with the ability to switch currencies and form…

5388c9a

…ats.

Bug Fixes for List.js and Table.js

063c916

List.js - getVisibleColumns was being imported from the wrong place Table.js - Selection would be 'all' and it would not be filterable. instead i have removed the filter if selection is all

Test Cases Fix for Existing Make Filter.

0dfcffd

dreadera requested review from adamdickinson, jarekwg, mwfister and uptickmetachu May 26, 2021 07:49

uptickmetachu reviewed May 27, 2021

View reviewed changes

adamdickinson suggested changes May 28, 2021

View reviewed changes

Update src/hooks/use-locale-number.ts

e22340f

Co-authored-by: Adam Dickinson <adam@renegadedigital.com.au>

jarekwg suggested changes May 28, 2021

View reviewed changes

dreadera changed the title ~~Typescript Support, Build Optimizations, Bug Fixes and Feature Request~~ feature: typescript support, build optimizations and currency support Jun 7, 2021

mwfister mentioned this pull request Jun 8, 2021

feature: replacing webpack with esbuild. uptick/react-object-table#36

Merged

dreadera closed this Jan 24, 2022

dreadera deleted the develop branch January 24, 2022 22:52

		@@ -0,0 +1,16 @@
		// Jest Snapshot v1, https://goo.gl/fbAQLP

		exports[`Currency Snapshots enders default 1`] = `

	describe('Convert Number To Locale', () => {
	describe('convert number to locale', () => {

feature: typescript support, build optimizations and currency support #125

feature: typescript support, build optimizations and currency support #125

Uh oh!

Conversation

dreadera commented May 26, 2021

The Problem

The Goal

What has been done

Uh oh!

uptickmetachu commented May 27, 2021

Uh oh!

uptickmetachu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dreadera commented May 27, 2021

Uh oh!

dreadera commented May 27, 2021

Uh oh!

adamdickinson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jarekwg left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jarekwg May 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dreadera commented May 28, 2021

Uh oh!

jarekwg commented May 28, 2021

Uh oh!

scaredcat commented Jun 1, 2021

Uh oh!

dreadera commented Jun 1, 2021

Uh oh!

Reviewers

Assignees

jarekwg left a comment •

edited

Loading

jarekwg May 28, 2021 •

edited

Loading