Migrating composables and a few vue component #1697

johnsimons · 2024-02-06T05:07:28Z

Migrating composables and a few vue component

Started adding typings for REST api resources

PhilBastian

in general i'd prefer to have the assignation to undefined and returning from functions of undefined changed to assigning/returning null, since using undefined is 'overloading' the default meaning of undefined as set by JavaScript of "this thing hasn't been defined, or this function has no return statement".

Comparisons to undefined should be comparisons to undefined or null (i.e. use the shortcut == null)

PhilBastian · 2024-02-06T06:30:16Z

src/ServicePulse.Host/vue/src/composables/formatter.js

-const hourDuration = moment.duration(60 * 1000); //this ensure that we never use minute formatting
-const dayDuration = moment.duration(24 * 60 * 60 * 1000);
-
-export function useFormatTime(value) {


these function's you haven't converted are used by monitoring and therefore will need to be converted

Ah, I thought I could take a shortcut 😄

So I converted all the functions now, but unsure about the function parameter types, I went with value: string, decimals: number. Is this right?

PhilBastian · 2024-02-06T06:46:23Z

src/ServicePulse.Host/vue/src/composables/serviceLicense.ts

-  const lic = await getLicense(license);
-  license = lic;
+  license = await getLicense();
  license.licenseEdition = computed(() => {


are these correct? Previously they held the reactive object that was the result of the computed, now they hold the value of the computed at the time of creation

I refactor all this code, tried using the spread operator but did not work either.
Also, the usage of this data, does not need .value, see https://github.com/Particular/ServicePulse/pull/1697/files#diff-97592647a6c6f377fdd850bdb6229985b9a414d6956fcc86213ff71f695b94d6

I tested it, and it works, but i would appreciate if you also have a look to make sure I am not introducing a bug here 😄

PhilBastian · 2024-02-06T07:03:52Z

src/ServicePulse.Host/vue/src/composables/serviceServiceControlUrls.ts


 export function useIsMonitoringDisabled() {
-  return monitoringUrl.value === null || monitoringUrl.value === "" || monitoringUrl.value === "!";
+  return monitoringUrl.value === undefined || monitoringUrl.value === "" || monitoringUrl.value === "!";


i think we want this first clause to be monitoringUrl.value == null, since it depends on how the form is handled whether the clearing of a value will result in an empty string or null, whereas it starts above as undefined.

PhilBastian · 2024-02-06T07:06:56Z

src/ServicePulse.Host/vue/src/composables/serviceServiceControlUrls.ts

+export function useFetchFromMonitoring(suffix: string) {
  if (useIsMonitoringDisabled()) {
-    return Promise.resolve(null);
+    return Promise.resolve(undefined);


I really don't like returning undefined from functions: should be null

PhilBastian · 2024-02-06T07:08:19Z

src/ServicePulse.Host/vue/src/composables/serviceServiceControlUrls.ts

+  const requestOptions: RequestInit = {
    method: "POST",
  };
  if (payload !== undefined) {


will also not want to add these headers if payload is null, so check should be if payload != null (i.e. it was wrong in the old version)

PhilBastian · 2024-02-06T07:22:32Z

src/ServicePulse.Host/vue/src/composables/serviceServiceControlUrls.ts

    console.debug(`ServiceControl Url found in QS and stored in local storage: ${serviceControlUrl.value}`);
  } else if (window.localStorage.getItem("scu")) {
-    serviceControlUrl.value = window.localStorage.getItem("scu");
+    serviceControlUrl.value = window.localStorage.getItem("scu") || undefined;


|| undefined doesn't make sense to me...? If the getItem doesn't find the key, won't it return undefined anyway?

PhilBastian · 2024-02-06T07:23:07Z

src/ServicePulse.Host/vue/src/composables/serviceServiceControlUrls.ts

  window.localStorage.removeItem("mu");

-  const newSearch = "?scu=" + newServiceControlUrl.value + "&mu=" + newMonitoringUrl.value;
+  let newSearch = "?scu=" + newServiceControlUrl.value + "&mu=" + newMonitoringUrl.value;


my bad, fixed

PhilBastian · 2024-02-06T07:25:09Z

src/ServicePulse.Host/vue/src/router/index.ts


 router.beforeEach((to, from, next) => {
-  document.title = to.meta.title || "ServicePulse";
+  document.title = `${to.meta.title} || "ServicePulse"`;


this is changing the title from what it previously was

Had to declare a definitions file, see https://router.vuejs.org/guide/advanced/meta.html#TypeScript

johnsimons

I think I addressed all comments @PhilBastian let me know

johnsimons · 2024-02-06T05:08:47Z

src/ServicePulse.Host/vue/src/components/__tests__/HelloWorld.spec.js

johnsimons · 2024-02-06T05:09:27Z

src/ServicePulse.Host/vue/src/components/TimeSince.vue


 onMounted(() => {
-  interval = setInterval(function () {
+  interval = window.setInterval(function () {


so it doesn't get confused with Node Timer, since we are also referencing the node definitions

johnsimons · 2024-02-06T05:11:13Z

src/ServicePulse.Host/vue/src/components/failedmessages/MessageView.vue

-  const textBody = await response.text();
-  failedMessage.value.messageBody = textBody;
+  try {
+    failedMessage.value.messageBody = await response.text();


I noticed this could throw when no body, so I added a catch

johnsimons · 2024-02-06T22:03:34Z

src/ServicePulse.Host/vue/src/router/index.ts


 router.beforeEach((to, from, next) => {
-  document.title = to.meta.title || "ServicePulse";
+  document.title = `${to.meta.title} || "ServicePulse"`;


johnsimons · 2024-02-06T22:08:09Z

src/ServicePulse.Host/vue/src/composables/serviceServiceControlUrls.ts


 export function useIsMonitoringDisabled() {
-  return monitoringUrl.value === null || monitoringUrl.value === "" || monitoringUrl.value === "!";
+  return monitoringUrl.value === undefined || monitoringUrl.value === "" || monitoringUrl.value === "!";


johnsimons · 2024-02-06T22:12:53Z

src/ServicePulse.Host/vue/src/composables/serviceServiceControlUrls.ts

+  const requestOptions: RequestInit = {
    method: "POST",
  };
  if (payload !== undefined) {


johnsimons · 2024-02-06T22:13:02Z

src/ServicePulse.Host/vue/src/composables/serviceServiceControlUrls.ts

    console.debug(`ServiceControl Url found in QS and stored in local storage: ${serviceControlUrl.value}`);
  } else if (window.localStorage.getItem("scu")) {
-    serviceControlUrl.value = window.localStorage.getItem("scu");
+    serviceControlUrl.value = window.localStorage.getItem("scu") || undefined;


johnsimons · 2024-02-06T22:13:39Z

src/ServicePulse.Host/vue/src/composables/serviceServiceControlUrls.ts

  window.localStorage.removeItem("mu");

-  const newSearch = "?scu=" + newServiceControlUrl.value + "&mu=" + newMonitoringUrl.value;
+  let newSearch = "?scu=" + newServiceControlUrl.value + "&mu=" + newMonitoringUrl.value;


my bad, fixed

johnsimons · 2024-02-06T22:56:27Z

src/ServicePulse.Host/vue/src/composables/serviceLicense.ts

-  const lic = await getLicense(license);
-  license = lic;
+  license = await getLicense();
  license.licenseEdition = computed(() => {


I refactor all this code, tried using the spread operator but did not work either.
Also, the usage of this data, does not need .value, see https://github.com/Particular/ServicePulse/pull/1697/files#diff-97592647a6c6f377fdd850bdb6229985b9a414d6956fcc86213ff71f695b94d6

I tested it, and it works, but i would appreciate if you also have a look to make sure I am not introducing a bug here 😄

johnsimons · 2024-02-06T23:33:05Z

src/ServicePulse.Host/vue/src/composables/formatter.js

-const hourDuration = moment.duration(60 * 1000); //this ensure that we never use minute formatting
-const dayDuration = moment.duration(24 * 60 * 60 * 1000);
-
-export function useFormatTime(value) {


Ah, I thought I could take a shortcut 😄

So I converted all the functions now, but unsure about the function parameter types, I went with value: string, decimals: number. Is this right?

johnsimons · 2024-02-06T23:37:33Z

src/ServicePulse.Host/vue/src/components/TimeSince.vue


-const props = defineProps({
-  dateUtc: String,
-  default: function () {


from reading the doco, I think this was wrong.
It should have been:

{ dateUtc: { type: String, default: ... }, }

anyway, I fixed it

johnsimons · 2024-02-06T23:38:52Z

src/ServicePulse.Host/vue/src/components/TimeSince.vue

+const props = withDefaults(defineProps<{ dateUtc: string }>(), { dateUtc: emptyDate });

-let interval = null;
+let interval: number | undefined = undefined;


I left this one as undefined @PhilBastian, mostly because the window.clearInterval takes a number or undefined

johnsimons · 2024-02-06T23:39:42Z

src/ServicePulse.Host/vue/src/composables/formatter.ts

+}
+
+function round(num: number, decimals: number) {
+  return Math.round(num + Number(`e+${decimals}`)) + Number(`e-${decimals}`);


I hope I got this right.
I wasn't sure why it needs to be this complex!

johnsimons · 2024-02-06T23:40:36Z

src/ServicePulse.Host/vue/src/composables/serviceLicense.ts

+  registered_to: "",
+  status: "",
+  license_status: LicenseStatus.Unavailable,
+  licenseEdition: computed(() => {


I decided to move the computed into the "empty/default" object, thoughts @PhilBastian ?

johnsimons · 2024-02-06T23:41:08Z

src/ServicePulse.Host/vue/src/composables/serviceSemVer.ts


 interface SemVer {
-  semver: string | undefined;
+  semver: string | null;


Going with null

PhilBastian · 2024-02-07T00:10:57Z

src/ServicePulse.Host/vue/src/composables/serviceServiceControlUrls.ts

-  }
-
-  return undefined;
+  return params.find((param) => {


does this still need to be null aware? i.e. params?.find((param) => param.name === key)

good question, not sure

The assumption here is that params should either be empty array or not, but not null.
I also checked the usages of this and they seem to always pass an array.

johnsimons requested review from PhilBastian and mikeminutillo February 6, 2024 05:07

johnsimons self-assigned this Feb 6, 2024

Migrating composables and a few vue component

d4744de

Started adding typings for REST api resources

johnsimons force-pushed the john/worm branch from 943a353 to d4744de Compare February 6, 2024 05:08

PhilBastian requested changes Feb 6, 2024

View reviewed changes

johnsimons added 5 commits February 7, 2024 08:17

Fixed router meta

3621b95

Had to declare a definitions file, see https://router.vuejs.org/guide/advanced/meta.html#TypeScript

Using nulls

d9ce78d

A few more fixes with nulls

5d19b69

Refactor based on feedback

a62a8f2

Added skipped functions back to formatter

be0a7fb

johnsimons commented Feb 6, 2024

View reviewed changes

johnsimons requested a review from PhilBastian February 6, 2024 23:41

PhilBastian approved these changes Feb 7, 2024

View reviewed changes

johnsimons merged commit 8e522c9 into master Feb 7, 2024

johnsimons deleted the john/worm branch February 7, 2024 02:12

mikeminutillo added the Type: Refactoring Type: Refactoring label Feb 7, 2024

mikeminutillo added this to the vNext milestone Feb 7, 2024

mikeminutillo mentioned this pull request Feb 9, 2024

Update Vue code to TypeScript #1710

Closed

Migrating composables and a few vue component #1697

Migrating composables and a few vue component #1697

Uh oh!

Conversation

johnsimons commented Feb 6, 2024

Uh oh!

PhilBastian 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!

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!

johnsimons 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!

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!