🔨 Update build tooling #26

sergei-maertens · 2023-04-20T09:41:33Z

Replaced unmaintained microbundle-crl with pure tsc compilation
Updated ESLint dependencies/rules
Removed unused react-scripts/jsdom tests (we'll use storybook test runner instead)
Fixed some eslint violations
Removed the example boilerplate
Removed react-scripts

* Replaced unmaintained microbundle-crl with pure tsc compilation * Updated ESLint dependencies/rules * Removed unused react-scripts/jsdom tests (we'll use storybook test runner instead) * Fixed some eslint violations * Removed the example boilerplate

.eslintrc

sergei-maertens · 2023-04-20T09:56:47Z

package.json

I updated this in line with other typescript based libraries, most notably formio-builder (which itself was inspired on design-token-editor and react-use).

Main takeaway is that bundling at the library level isn't particularly needed, especially because we use webpack in the final projects anyway which takes care of bundling.

ideally, downstream projects would use the esm dist files, which maximizes the potential for tree-shaking/stripping out unused modules.

package.json

src/components/textfield/textfield.component.tsx

tsconfig.test.json

.eslintrc

package.json

src/lib/validation/validate.ts

svenvandescheur · 2023-04-20T15:20:23Z

src/lib/validation/validate.ts

@@ -74,6 +74,12 @@ export const getFormErrors = async (
  }
 };

+function isScalarValue(obj: IValues | Value | Values): obj is Value {


Isn't this just a primitive (more common name) check? Can't we simplify this too: const isPrimitive = (value) => value !== Object(value)?

the point of this function is that it provides type narrowing without casting it be a particular type, otherwise there are type errors due to the arbitrary JSON structure of the submission data.

There are some more TODO's because a leaf node/form field is not actually guaranteed to be a scalar/primitive, they may also be objects or arrays, which we need to tackle later. So for now this only exists to make the code properly compile.

I see the purpose of the function here but I still have concerns about the confusing naming (primitive should be name here) and its implementation. The "temporary" nature doesn't not change that aspect. Also: being a todo I think it should be either commented or an issue should be created for it.

There is actually a meaningful difference - scalar is the opposite from compounded (like arrays, objects...) and is intended to refer to actual, concrete values. From a mathematical point of view this is more correct, since primitives in JS also include symbols, which is most definitely not supported as a value.

I've created #27 to keep track of this.

There already is a FIXME in the code: https://github.com/open-formulieren/formio-renderer/pull/26/files/a93bd447fd6ac9d2201ba2f2416159cb55e90894#diff-84df24a17f5d119e036317e16e51ce0afd5fe35628a93e15f0d32ff6f2c7d41fR117

This pull requests shouldn't have been merged without approval. Independently of the semantic discussion whether we should adhere to math or js concepts. I have some concerns regarding the implementation which should have been resolved through the proper process.

I merged it because it's fixing the (reportedly) broken build chain, which was blocking any other feature and preventing us from using the formio utils which are known to have a correct implementation and our own implementation was undeniably insufficient

I shall adress the remaining comments tomorrow, either through justification as a comment or a follow up PR to correct them.

Renamed the function to isSingleValue because that's essentially what the type predicate expresses too. As added in the comments, this is still not 100% correct but that requires tweaking the Value types/interfaces extensively. I'm really thinking we should be able to map the static formio component types to the matching value types and use some typescript generics magic get specific types instead of having to deal with mismatching types (e.g. boolean for textfield does not make any sense).

svenvandescheur · 2023-04-24T08:05:43Z

src/lib/validation/validate.ts

@@ -74,6 +74,12 @@ export const getFormErrors = async (
  }
 };

+function isScalarValue(obj: IValues | Value | Values): obj is Value {


Is there a reason why this is a regular function and not an arrow function adhering to the surrounding coding style?

Yeah, had to look up exactly how to do type predicates with arrow functions and when I figured it out I found it completely unreadable.

See also https://stackoverflow.com/a/47847949

Rewrote it as an arrow function in #29

As an aside, there are other cases where correctly typing React components with arrow functions and generics is not possible, where using a function declaration` is the only option. I also kind of fail the see why every function should be defined as an arrow function when plain functions work equally well and have benefits from being hoisted and being easier syntax to read, especially in combination with the syntax.

svenvandescheur · 2023-04-24T08:10:08Z

src/lib/validation/validate.ts

@@ -74,6 +74,12 @@ export const getFormErrors = async (
  }
 };

+function isScalarValue(obj: IValues | Value | Values): obj is Value {
+  if (Array.isArray(obj)) return false;


This seems redundant as typeof [] is object?

Yes, I had the logic the other way around first and forgot to clean this up. Removed it in #29

There is some nuance where we might have to detect whether a value is an array or not, but that should rely on component.multiple rather than deriving it from untrusted value input.

svenvandescheur · 2023-04-24T08:10:21Z

src/lib/validation/validate.ts

@@ -74,6 +74,12 @@ export const getFormErrors = async (
  }
 };

+function isScalarValue(obj: IValues | Value | Values): obj is Value {
+  if (Array.isArray(obj)) return false;
+  if (obj == null) return true;


Can this be a strict comparison?

Yep, I was thrown off by the convention that foo == null catches both null and undefined, but in this instance that's not relevant since typeof undefined is undefined so we only need to check for null explicitly. Addressed in #29

* Rewrote and renamed the isScalarValue type predicate as arrow func * Added more TODOs and FIXMEs regarding the component value types * Made null check strict, as typeof undefined is undefined and not object * Removed redundant Array.isArray check

…eedback-leftovers 👌 [#26] -- addressed remaining PR feedback

sergei-maertens requested a review from svenvandescheur April 20, 2023 09:41

🔨 Update build tooling

a93bd44

* Replaced unmaintained microbundle-crl with pure tsc compilation * Updated ESLint dependencies/rules * Removed unused react-scripts/jsdom tests (we'll use storybook test runner instead) * Fixed some eslint violations * Removed the example boilerplate

sergei-maertens force-pushed the chore/update-build-configuration branch from 59c1b76 to a93bd44 Compare April 20, 2023 09:43

sergei-maertens commented Apr 20, 2023

View reviewed changes

.eslintrc Show resolved Hide resolved

sergei-maertens commented Apr 20, 2023

View reviewed changes

package.json Show resolved Hide resolved

sergei-maertens commented Apr 20, 2023

View reviewed changes

package.json Show resolved Hide resolved

sergei-maertens commented Apr 20, 2023

View reviewed changes

src/components/textfield/textfield.component.tsx Show resolved Hide resolved

sergei-maertens commented Apr 20, 2023

View reviewed changes

tsconfig.test.json Show resolved Hide resolved

svenvandescheur requested changes Apr 20, 2023

View reviewed changes

sergei-maertens merged commit 063f087 into main Apr 21, 2023

svenvandescheur reviewed Apr 24, 2023

View reviewed changes

svenvandescheur added a commit that referenced this pull request May 1, 2023

Merge pull request #29 from open-formulieren/chore/build-tooling-pr-f…

d51cf3f

…eedback-leftovers 👌 [#26] -- addressed remaining PR feedback

sergei-maertens deleted the chore/update-build-configuration branch May 10, 2023 19:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🔨 Update build tooling #26

🔨 Update build tooling #26

sergei-maertens commented Apr 20, 2023 •

edited

Loading

sergei-maertens Apr 20, 2023

svenvandescheur Apr 20, 2023

sergei-maertens Apr 21, 2023 •

edited

Loading

svenvandescheur Apr 21, 2023

sergei-maertens Apr 21, 2023 •

edited

Loading

svenvandescheur Apr 24, 2023

sergei-maertens Apr 24, 2023

sergei-maertens Apr 30, 2023

svenvandescheur Apr 24, 2023

sergei-maertens Apr 24, 2023

sergei-maertens Apr 30, 2023

svenvandescheur Apr 24, 2023

sergei-maertens Apr 30, 2023 •

edited

Loading

svenvandescheur Apr 24, 2023

sergei-maertens Apr 30, 2023

🔨 Update build tooling #26

🔨 Update build tooling #26

Conversation

sergei-maertens commented Apr 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergei-maertens Apr 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergei-maertens Apr 21, 2023 • 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

sergei-maertens Apr 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergei-maertens commented Apr 20, 2023 •

edited

Loading

sergei-maertens Apr 21, 2023 •

edited

Loading

sergei-maertens Apr 21, 2023 •

edited

Loading

sergei-maertens Apr 30, 2023 •

edited

Loading