feat: [M3-7806] - Linode Create Refactor - Part 1 by bnussman-akamai · Pull Request #10268 · linode/manager

bnussman-akamai · 2024-03-08T20:17:44Z

Description 📝

This begins the refactor of the Linode Create flow. The goal is to build a new create flow from scratch using modern practices so that the Linode create flow is better and more maintainable. The goal is for this new create flow to be "backwards compatible", meaning we can flip the feature flags and customers have the exact same experience.

Initial PR of the Linode Create Refactor 🎉
- Introduces react-hook-form 🔘
  - We are doing this for the type-safety, performance, and flexibility (in comparison to formik)
- Setups up initial scaffolding and feature flag 🚩
- Begins incorporating some existing components (region select and plan selection)

Preview 📷

Screen.Recording.2024-03-08.at.3.14.17.PM.mov

How to test 🧪

Check out this PR
Use our local dev tools 🔧 and enable the new create flow feature flag
Test the new create flow
Look at my code and be critical
Verify I didn't break anything with the existing Linode create flow components

As an Author I have considered 🤔

bnussman-akamai · 2024-03-08T20:23:30Z

packages/manager/src/features/Linodes/LinodesCreate/LinodeCreate.tsx

I'm going to make it my goal to not touch the old create flow as I build out this new one, but I might make some exceptions

Agreed! That's ok, but also we could go without those to tighten scope and have reviewers only test the new flow.

Just like the type change, It makes things more complicated to ship/merge since we have to worry about production code being affected and not being behind the flag. It's such a crucial flow we want to be a bit strict about it.

Yeahhh. My original intention was to do exactly that.

I wanted to do the type change (CreateLinodeRequest) because either way it should to be fixed for the sake of correctness and consumers of @linode/api-v4

Going forward I'll force myself to be more strict. I'm not too stressed about breaking the existing flow because we have some testing coverage for it, but it definitely could still happen.

I still feel like this could go in its own PR, but if not, can we at least get two changesets?

one upcoming for v2 refactor

one change for APIv4

bnussman-akamai · 2024-03-08T20:23:52Z

packages/manager/src/features/Linodes/LinodeCreatev2/index.tsx

This is the root of the new create flow!

bnussman-akamai · 2024-03-08T20:25:19Z

packages/api-v4/src/linodes/types.ts

+  type: string;
+  region: string;


This was a major oversight. type and region are the two required values to create a Linode

😮 thx for catching

Did you consider creating a v2 type and not touch at all the existing flow?

We could do that, but I think it's fair to fix the primary type.

bnussman-akamai · 2024-03-08T20:35:05Z

packages/manager/src/features/Linodes/LinodesCreate/LinodeCreate.tsx

+          <Stack gap={3}>
+            <Tabs


I just added a Stack so that we could remove the margin from each component and control the spacing with a Stack in the new create flow.

github-actions · 2024-03-08T20:53:33Z

Coverage Report: ✅
Base Coverage: 81.39%
Current Coverage: 81.39%

bnussman-akamai · 2024-03-08T21:50:56Z

packages/manager/src/features/Linodes/LinodeCreatev2/index.tsx

+          <Error />
+          <Region />
+          <Plan />
+          <Summary />


I'm not sure how well these component names will scale, but for now, I'm just trying to keep things dead simple. We can adjust as we build this out

abailly-akamai

So nice! Super clean implementation so far and nice to see the introduction of React Forms.

Can we consider not modifying the existing flow at all?

abailly-akamai · 2024-03-11T17:26:11Z

packages/api-v4/src/linodes/types.ts

+  type: string;
+  region: string;


😮 thx for catching

Did you consider creating a v2 type and not touch at all the existing flow?

abailly-akamai · 2024-03-11T17:31:19Z

packages/manager/src/features/Linodes/LinodeCreatev2/Plan.tsx

+  const regionId = watch('region');
+
+  const hasCreateLinodePermission =
+    grants === undefined || grants.global.add_linodes;


there are new hooks to handle those permissions. useIsResourceRestricted & useRestrictedGlobalGrantCheck. am thinking we may want to start/keep using those for consistency

Ahh yes. Thank you! I couldn't remember the names of these!

abailly-akamai · 2024-03-11T17:35:18Z

packages/manager/src/features/Linodes/LinodeCreatev2/Plan.tsx

+  const { watch } = useFormContext<CreateLinodeRequest>();
+  const { field, fieldState } = useController<CreateLinodeRequest>({
+    name: 'type',
+  });


this looks like a very useful API - will learn about useController more

abailly-akamai · 2024-03-11T17:40:22Z

packages/manager/src/features/Linodes/LinodeCreatev2/Error.tsx

+      </Notice>
+    </Paper>
+  );
+};


Love it - can formState.errors be an array of errors?

I don't think formState.errors would ever be an array. errors will match the shape of the form type (CreateLinodeRequest in this case).

For example formState.errors.interfaces?.[0]?.ipv4?.nat_1_1?.message is valid and typesafe, so arrays are supported. This is great because formik supported arrays, but struggled with getting the type safety correct.

abailly-akamai · 2024-03-11T17:42:39Z

packages/manager/src/features/Linodes/LinodesCreate/LinodeCreate.tsx

Agreed! That's ok, but also we could go without those to tighten scope and have reviewers only test the new flow.

Just like the type change, It makes things more complicated to ship/merge since we have to worry about production code being affected and not being behind the flag. It's such a crucial flow we want to be a bit strict about it.

…quest`

jaalah-akamai · 2024-03-12T17:26:19Z

packages/manager/src/features/Linodes/LinodeCreatev2/index.tsx

+
+  return (
+    <FormProvider {...methods}>
+      <form onSubmit={methods.handleSubmit(onSubmit)}>


I get why you named it onSubmit, it just departs from our convention of naming the handler handleSubmit... maybe we need to revise what we do

jaalah-akamai

Great comments @abailly-akamai - This is an amazing start @bnussman and is looking clean! 👍

linode create v2 initial commit

e441cc7

bnussman-akamai self-assigned this Mar 8, 2024

fix old linode create flow spacing

29a1483

bnussman-akamai commented Mar 8, 2024

View reviewed changes

bnussman added 2 commits March 8, 2024 15:27

Merge branch 'develop' into linode-create-v2-part-1

175cc34

Added changeset: Linode Create Refactor - Part 1

f6840cf

bnussman-akamai marked this pull request as ready for review March 8, 2024 20:31

bnussman-akamai requested review from a team as code owners March 8, 2024 20:31

bnussman-akamai requested review from cliu-akamai, jaalah-akamai and mjac0bs and removed request for a team March 8, 2024 20:31

bnussman-akamai added the Linode Create Refactor label Mar 8, 2024

bnussman-akamai commented Mar 8, 2024

View reviewed changes

clean up comments

10256b8

bnussman-akamai requested review from abailly-akamai and hkhalil-akamai March 8, 2024 20:58

bnussman-akamai added the Ready for Review label Mar 8, 2024

bnussman-akamai commented Mar 8, 2024

View reviewed changes

abailly-akamai reviewed Mar 11, 2024

View reviewed changes

bnussman added 2 commits March 11, 2024 14:38

use useRestrictedGlobalGrantCheck helpers

b7c35c9

Added changeset: Make type and region required in `CreateLinodeRe…

f090e5c

…quest`

bnussman-akamai requested a review from abailly-akamai March 11, 2024 21:48

abailly-akamai approved these changes Mar 12, 2024

View reviewed changes

jaalah-akamai reviewed Mar 12, 2024

View reviewed changes

jaalah-akamai approved these changes Mar 12, 2024

View reviewed changes

bnussman-akamai removed the Ready for Review label Mar 12, 2024

bnussman-akamai added the Approved Multiple approvals and ready to merge! label Mar 12, 2024

Merge branch 'develop' into linode-create-v2-part-1

f1f7e8a

bnussman-akamai merged commit aa59941 into linode:develop Mar 12, 2024

This was referenced Mar 18, 2024

Release v1.115.0 - release → staging #10292

Merged

Release v1.115.0 - staging → master #10293

Merged

Conversation

bnussman-akamai commented Mar 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description 📝

Preview 📷

How to test 🧪

As an Author I have considered 🤔

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!

bnussman-akamai Mar 8, 2024 • 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!

github-actions bot commented Mar 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnussman-akamai Mar 8, 2024 • 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!

abailly-akamai 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!

bnussman-akamai Mar 11, 2024 • 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!

jaalah-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bnussman-akamai commented Mar 8, 2024 •

edited

Loading

bnussman-akamai Mar 8, 2024 •

edited

Loading

github-actions bot commented Mar 8, 2024 •

edited

Loading

bnussman-akamai Mar 8, 2024 •

edited

Loading

bnussman-akamai Mar 11, 2024 •

edited

Loading