Conversation
f517279 to
1a146e9
Compare
1bd0db6 to
9911294
Compare
Now null is expected instead of undefined
For ensuring code works as expectd and allow refactor it with more confidence.
Instead of the custom findDeviceIndex. The radashi method already covered the undefined/nullish corner cases well.
- Adapt UI to HTTP API v2. - New controllers section. - New devices table. - New controllers page for activating controllers.
- And allow configuring zFCP in the empty state.
e6bf7e8 to
34b404f
Compare
dgdavid
left a comment
There was a problem hiding this comment.
Thanks a ton for taking care of this 🙏 I know it has been a little headache 😓, but the result will pay-off. I have to check to minor things before merging it, reason why I'm postponing the approval.
|
|
||
| /** Generates a copy of the given config or a default config. */ | ||
| function ensureConfig(config: Config | null): Config { | ||
| return config ? { ...config } : { ...DEFAULT_CONFIG }; |
There was a problem hiding this comment.
well, I would probably use StructuredClone as it is already enough supported - https://caniuse.com/?search=structuredClone
There was a problem hiding this comment.
As said in IRC, let's postpone such a big decision until we have more time to play and learn about such a method. I prefer to be a bit conservative here 🙏
From MDN.
The method also allows transferable objects in the original value to be transferred rather than cloned to the new object. Transferred objects are detached from the original object and attached to the new object; they are no longer accessible in the original object.
https://developer.mozilla.org/en-US/docs/Web/API/Window/structuredClone
|
|
||
| /** Generates a copy of the given config or a default config. */ | ||
| function ensureConfig(config: Config | null): Config { | ||
| return config ? { ...config } : { ...DEFAULT_CONFIG }; |
There was a problem hiding this comment.
also what about nullish operator? So something like
return structuredClone(object ?? DEFAULT_CONFIG)
|
|
||
| /** Generates a copy of the given config or a default config. */ | ||
| function ensureConfig(config: Config | null): Config { | ||
| return config ? { ...config } : { ...DEFAULT_CONFIG }; |
There was a problem hiding this comment.
here is link why destructuring is not enough if object contain array - https://runjs.app/play/#LyoKKiDwn5GLIFdlbGNvbWUgdG8gdGhlIG9ubGluZSBKYXZhU2NyaXB0IHBsYXlncm91bmQg8J+agAoqCiogVG8gZ2V0IHN0YXJ0ZWQsIHRyeSB3cml0aW5nIHNvbWUgY29kZQoqCiogRm9yIGVhY2ggZXhwcmVzc2lvbiB5b3Ugd3JpdGUsIHlvdSdsbCBzZWUgdGhlIHJlc3VsdCBpbiB0aGUgb3V0cHV0IHBhbmVsCiovCgonSGVsbG8sIFdvcmxkISDwn4yOJzsKCjUgKiA1OwoKYXdhaXQgUHJvbWlzZS5yZXNvbHZlKCdUb3AtbGV2ZWwgYXdhaXQg8J+kqScpOwoKLyoKKiBHaXZlIGl0IGEgdHJ5IPCfmIAgCiovCgpsZXQgYSwgYiwgcmVzdDsKYSA9IHsKICAidGVzdCI6IFsxLCAyLCAzXQp9CgpiID0geyAuLi5hIH07Cgpjb25zb2xlLmxvZyhhKTsKCmNvbnNvbGUubG9nKGIpOwoKYlsidGVzdCJdLnB1c2goNCk7Cgpjb25zb2xlLmxvZyhhKTsKCmNvbnNvbGUubG9nKGIpOwoK
There was a problem hiding this comment.
I see your point. However, I think in our case this shouldn't be a concern because we're not destructuring objects in a way that would lead to this situation. The pattern we're using is slightly different: with each new copy we're "replacing the previous state" rather than mutating or reusing it. Not sure if I'm explaining it well.
So while it's true that destructuring only creates a shallow copy (and arrays or nested objects will still share references), that scenario shouldn't occur with how we're handling the data here.
That said, it's always good to stay vigilant about these edge cases and improve things, so thanks for pointing it out 👍
dgdavid
left a comment
There was a problem hiding this comment.
I've sent aesthetic changes, find screenshots attached below. Let me know if you find them to divergent with your view. If not, you can go ahead with the merge. I think the recursive fix can be postponed for another round of improvements, but up to you to go for the reducer proposed by @jreidinger before merging. No strong opinion
Prepare to release version 19. * #1829 * #2508 * #2772 * #2818 * #2826 * #2848 * #2860 * #2863 * #2864 * #2866 * #2867 * #2869 * #2870 * #2871 * #2872 * #2873 * #2874 * #2875 * #2876 * #2877 * #2880 * #2881 * #2882 * #2884 * #2885 * #2886 * #2891 * #2892 * #2893 * #2894 * #2895 * #2896 * #2897 * #2898 * #2899 * #2900 * #2901 * #2902 * #2903 * #2904 * #2908 * #2909 * #2910 * #2912 * #2913 * #2914 * #2915 * #2916 * #2917 * #2918 * #2920 * #2921 * #2923 * #2924 * #2926 * #2928 * #2929 * #2930 * #2933 * #2934 * #2935 * #2936 * #2937 * #2938 * #2939 * #2942 * #2943 * #2944 * #2945 * #2946 * #2947 * #2948 * #2949 * #2950 * #2951 * #2952 * #2954 * #2955 * #2956 * #2957 * #2958 * #2959 * #2960 * #2961 * #2963 * #2964 * #2965 * #2967 * #2968 * #2969 * #2970 * #2971 * #2972 * #2974 * #2975 * #2977 * #2978 * #2980 * #2981 * #2982 * #2983 * #2984 * #2988 * #2989 * #2990 * #2991 * #2992 * #2993 * #2994 * #2995 * #2996 * #2997 * #2998 * #2999 * #3000 * #3001 * #3002 * #3004 * #3005 * #3006 * #3007 * #3008 * #3009 * #3011 * #3012 * #3013 * #3014 * #3015 * #3016 * #3018 * #3019 * #3020 * #3021 * #3022 * #3023 * #3024 * #3025 * #3026 * #3027 * #3028 * #3029 * #3030 * #3031 * #3033 * #3034 * #3035 * #3036 * #3037 * #3039 * #3040 * #3041 * #3042 * #3043 * #3044 * #3045 * #3046 * #3047 * #3048 * #3049 * #3050 * #3051 * #3052 * #3053 * #3054 * #3055 * #3056 * #3057 * #3058 * #3060 * #3061 * #3062 * #3063 * #3064 * #3065 * #3066 * #3067 * #3068 * #3069 * #3070 * #3071 * #3072 * #3073 * #3074 * #3075 * #3076 * #3077 * #3078 * #3079 * #3086 * #3087 * #3088 * #3089 * #3090 * #3091 * #3092 * #3093 * #3094 * #3095 * #3096 * #3097 * #3098 * #3099 * #3100 * #3101 * #3102 * #3103 * #3104 * #3105 * #3106 * #3107 * #3108 * #3109 * #3110 * #3112 * #3113 * #3114 * #3115 * #3116 * #3117 * #3118 * #3119 * #3120 * #3122 * #3123 * #3124 * #3127 * #3128 * #3129 * #3130 * #3131 * #3133 * #3134 * #3135 * #3136 * #3137 * #3138 * #3139 * #3140 * #3141 * #3142 * #3143 * #3144 * #3145 * #3146 * #3147 * #3148 * #3149 * #3150 * #3151 * #3152 * #3153 * #3154 * #3155 * #3157 * #3158 * #3159 * #3160 * #3161 * #3162 * #3163 * #3164 * #3165 * #3166 * #3167 * #3168 * #3169 * #3170 * #3174 * #3175 * #3176 * #3177 * #3178 * #3179 * #3181 * #3182 * #3184 * #3185 * #3186 * #3188 * #3189 * #3190 * #3191 * #3192 * #3194 * #3195 * #3196 * #3197 * #3198 * #3199 * #3200 * #3201 * #3202 * #3203 * #3205 * #3206 * #3208 * #3209 * #3210 * #3213 * #3214 * #3215 * #3216 * #3217 * #3218 * #3219 * #3220 * #3222 * #3223 * #3224 * #3225 * #3226 * #3227 * #3228 * #3229 * #3230 * #3231 * #3232 * #3233 * #3234 * #3235 * #3236 * #3237 * #3238 * #3239 * #3240 * #3241 * #3242 * #3243 * #3244 * #3246 * #3247 * #3248 * #3250 * #3251 * #3252 * #3253 * #3254 * #3255 * #3256 * #3257 * #3258 * #3259 * #3260 * #3261 * #3262 * #3263 * #3265 * #3266 * #3267 * #3268 * #3269 * #3270 * #3271 * #3272 * #3273 * #3274 * #3275 * #3276 * #3277 * #3278 * #3279 * #3280 * #3281 * #3282 * #3283 * #3284 * #3285 * #3286 * #3287 * #3288 * #3289 * #3290 * #3291
Revamp the zFCP UI: adapt to HTTP API v2 and follow the same patterns as rest of the UI.