chore: add ui-components package #2321

katelynsills · 2021-02-03T02:04:37Z

Add ui-components package to agoric-sdk, with tests.

Still to do:

Run tests successfully under SES (currently errors, see below)
Use @agoric/ertp in the component (needed to be run under SES)
Copy over the actual display.js, not the mock here (needs to be run under SES because uses @agoric/assert)
Use Bigints
Further test functionality by replacing React components in dapp-token-economy

Closes: #2320

michaelfig · 2021-02-03T19:03:25Z

Removing intrinsics.Object.assign.getPolyfill
Removing intrinsics.Object.assign.implementation
Removing intrinsics.Object.assign.shim
Removing intrinsics.Object.values.getPolyfill
Removing intrinsics.Object.values.implementation
Removing intrinsics.Object.values.shim

These messages are likely describing the source of the errors. SES is finding nonstandard properties on Object.assign and Object.values.

We don't want Babel to polyfill anything, IIUC.

Can you disable core-js if that is being used? It has bad interactions with SES.

katelynsills · 2021-02-03T22:31:54Z

Removing intrinsics.Object.assign.getPolyfill
Removing intrinsics.Object.assign.implementation
Removing intrinsics.Object.assign.shim
Removing intrinsics.Object.values.getPolyfill
Removing intrinsics.Object.values.implementation
Removing intrinsics.Object.values.shim

These messages are likely describing the source of the errors. SES is finding nonstandard properties on Object.assign and Object.values.

We don't want Babel to polyfill anything, IIUC.

Can you disable core-js if that is being used? It has bad interactions with SES.

Thanks for the tip! Let me see what I can do

katelynsills · 2021-02-04T02:39:12Z

There are two separate issues here:

enzyme, AVA's recommended react testing library is trying to add properties to Object.assign and Object.values.
A dependency ofmaterial-ui/core is trying to get globalThis without using globalThis. It returns undefined under SES, and a few lines later, the code tries to get a property of undefined and errors.

(I had forgotten that I had Enzyme setup code running before each test, so that explains how Enzyme was able to execute before lockdown.)

katelynsills · 2021-02-05T00:19:44Z

External PR to change the jss package which MaterialUI depends on: cssinjs/jss#1449

katelynsills · 2021-02-10T23:12:37Z

Update as of 2/10/2021:

The bug in JSS which was prohibiting us from using MaterialUI under SES, has been merged into JSS master.. Once a new release of JSS is made, we should be able to upgrade jss locally and our copy of MaterialUI should use it. We should also probably ask MaterialUI to upgrade their dependency, as people who have already installedMaterialUI with a lockfile will have to manually upgrade otherwise to use SES.
The bug in enzyme and its dependency object.assign was resolved in the latest version of object.assign. This version of object.assign is not yet in the published version of enzyme, but that should be ok for our uses. The only people disadvantaged are those who have already installed enzyme with a lockfile - people installing enzyme anew will get the latest version of object.assign.

With these two fixes, my tests pass. I'm going to put this on the back-burner to work on ERTP Beta tasks, but will come back to this once JSS gets a new release, or if we reprioritize this.

katelynsills · 2021-03-17T23:27:26Z

The new version of JSS has been released, and tests now pass on Node 14! (have not tried Node 15).

katelynsills · 2021-03-19T22:45:35Z

@michaelfig, I think the NatAmountInput itself is good, with correct decimalPlaces handling and tests. However, I'm having trouble importing it in dapp-token-economy, especally since dapp-token-economy isn't ejected. I am wondering if we can merge this in even if I can't get the packaging/import to work yet.

dtribble

Minor optional comments. GREAT tests.

packages/ui-components/src/display/display.js

dtribble · 2021-03-20T04:27:12Z

packages/ui-components/src/display/display.js

+ * @param {number=} decimalPlaces - places to move the decimal to the left
+ * @returns {Value}
+ */
+export const parseAsValue = (


why clutter the name with As? parseValue says what I want it to do and what I'm expecting.

To me, it wasn't clear in parseX whether x was the input type or the output type. In parseValue, value is the output type, but that doesn't match how we usually say it in English: "parse this string as a value". In English, the input directly follows parse, and the output is prefixed with as. So to make the name clearer since it is the opposite of how we would speak normally, I added the As.

dtribble · 2021-03-20T04:28:41Z

packages/ui-components/src/display/display.js

+ * @param {number=} decimalPlaces - places to move the decimal to the left
+ * @returns {Amount}
+ */
+export const parseAsAmount = (


Unless it's well motivated, the As is also an unnecessary renaming over what's there, resulting in merge overhead. Of course it might be well-motivated.

packages/ui-components/src/display/display.js

packages/ui-components/src/display/natValue/parseAsNat.js

packages/ui-components/src/display/natValue/roundToDecimalPlaces.js

michaelfig · 2021-03-20T08:12:30Z

I am wondering if we can merge this in even if I can't get the packaging/import to work yet.

I'm happy for you to make forward progress by merging this, and deal with the finicky (mostly unrelated to your PR) import/export issues later. I may get to review this on the weekend, but don't feel obligated to wait for me.

michaelfig

Just a few suggestions. Take or leave them.

packages/ui-components/README.md

packages/ui-components/jsconfig.json

packages/ui-components/README.md

michaelfig · 2021-03-21T17:51:11Z

packages/ui-components/package.json

+  "name": "ui-components",
+  "version": "0.0.1",
+  "description": "Reusable UI Components for Agoric Dapps, built with React and MaterialUI",
+  "main": "dist/index.js",


This is probably the source of the import problems. React components can't easily be imported from bundles, IIUC. The build step has to be performed only once, in the app that imports all the other components. Otherwise, your app can't tree-shake the components that it doesn't use that are already bundled with other things.

If this change doesn't fix it for you, you might have instead to import the components as import NatAmountInput from '@agoric/ui-components/src/components/NatAmountInput'

Suggested change

"main": "dist/index.js",

"main": "src/index.js",

I tried using the src directly, and it doesn't work in dapp-token-economy. I get an error message telling me to change my Babel settings to allow for JSX, but since we haven't ejected, I haven't found an easy way to change the Babel settings :/ There must be a way to bundle it correctly, though, since we can import MaterialUI components from packages.

packages/ui-components/package.json

michaelfig · 2021-03-21T18:01:23Z

packages/ui-components/test/components/test-NatAmountInput.js

+// @ts-ignore path is correct for compiled output
+import { NatAmountInput } from '../../../dist'; // eslint-disable-line import/no-unresolved


Can you please try this out? With the "main" change in package.json this should always work.

Suggested change

// @ts-ignore path is correct for compiled output

import { NatAmountInput } from '../../../dist'; // eslint-disable-line import/no-unresolved

import { NatAmountInput } from '../../..';

I don't think this will work, because the source needs to be compiled for the tests to run. I am compiling manually through babel.

Tested and confirmed that the compiled version is necessary because of the JSX:

Uncaught exception in compiled/test/components/test-NatAmountInput.js /Users/katesills/code/agoric-sdk/packages/ui-components/src/components/NatAmountInput.js:31 <TextField ^ SyntaxError: Invalid or unexpected token

katelynsills requested a review from michaelfig February 3, 2021 02:04

katelynsills self-assigned this Feb 3, 2021

katelynsills added the Dapp & UI Support label Feb 9, 2021

erights mentioned this pull request Feb 11, 2021

Tracking issue for getting 3rd party packages more SES friendly endojs/endo#576

Closed

katelynsills added the Beta label Feb 17, 2021

katelynsills added this to the Beta Launch milestone Feb 17, 2021

katelynsills force-pushed the 2320-ui-components branch from d4f76a8 to 279ea22 Compare March 17, 2021 23:25

katelynsills force-pushed the 2320-ui-components branch 2 times, most recently from 21f6983 to a321a7b Compare March 19, 2021 21:33

katelynsills marked this pull request as ready for review March 19, 2021 22:43

katelynsills requested a review from dtribble March 19, 2021 22:45

katelynsills force-pushed the 2320-ui-components branch from 3951bc8 to 76d1a31 Compare March 19, 2021 22:51

dtribble approved these changes Mar 20, 2021

View reviewed changes

michaelfig approved these changes Mar 21, 2021

View reviewed changes

katelynsills added 3 commits March 22, 2021 12:55

feat: add NatAmountInput

4f59964

chore: fix README

3781e5b

chore: address PR comments

3d5eede

katelynsills force-pushed the 2320-ui-components branch from e3f131c to 3d5eede Compare March 22, 2021 20:25

chore: move helpers

dab069e

katelynsills enabled auto-merge (squash) March 22, 2021 20:39

katelynsills merged commit 770542e into master Mar 22, 2021

katelynsills deleted the 2320-ui-components branch March 22, 2021 20:44

kriskowal mentioned this pull request Feb 6, 2024

Ecosystem Compatibility (2024-02-05 onward) endojs/endo#2037

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: add ui-components package #2321

chore: add ui-components package #2321

katelynsills commented Feb 3, 2021 •

edited

Loading

michaelfig commented Feb 3, 2021

katelynsills commented Feb 3, 2021

katelynsills commented Feb 4, 2021

katelynsills commented Feb 5, 2021

katelynsills commented Feb 10, 2021

katelynsills commented Mar 17, 2021

katelynsills commented Mar 19, 2021

dtribble left a comment

dtribble Mar 20, 2021

katelynsills Mar 22, 2021

dtribble Mar 20, 2021

michaelfig commented Mar 20, 2021

michaelfig left a comment

michaelfig Mar 21, 2021

katelynsills Mar 22, 2021

michaelfig Mar 21, 2021

katelynsills Mar 22, 2021

katelynsills Mar 22, 2021

		// @ts-ignore path is correct for compiled output
		import { NatAmountInput } from '../../../dist'; // eslint-disable-line import/no-unresolved

chore: add ui-components package #2321

chore: add ui-components package #2321

Conversation

katelynsills commented Feb 3, 2021 • edited Loading

michaelfig commented Feb 3, 2021

katelynsills commented Feb 3, 2021

katelynsills commented Feb 4, 2021

katelynsills commented Feb 5, 2021

katelynsills commented Feb 10, 2021

katelynsills commented Mar 17, 2021

katelynsills commented Mar 19, 2021

dtribble left a 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

michaelfig commented Mar 20, 2021

michaelfig left a 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

katelynsills commented Feb 3, 2021 •

edited

Loading