#4367: Add support for spaces in "Copy Property Path" #4371

fregante · 2022-09-29T05:14:25Z

What does this PR do?

Fixes “Copy Property Path” generates invalid paths to properties for fields that contain spaces #4367

Discussion

It looks like the path is being handled by lodash.toPath, I added tests to ensure that the result of this stringification is turned back into the original array by _.toPath

Demo

Screen.Recording.2.mov

Future work

Do these also need to be updated/replaced?

pixiebrix-extension/src/utils/debugUtils.ts

Line 31 in 3cce78f

export function getInvalidPath(

pixiebrix-extension/src/utils.ts

Lines 90 to 93 in 3cce78f

    
           export function joinPathParts(...nameParts: Array<string | number>): string { 
        
             // Don't use lodash.compact and lodash.isEmpty since they treat 0 as falsy 
        
             return nameParts.filter((x) => x != null && x !== "").join("."); 
        
           }

Checklist

Add tests
Designate a primary reviewer: @twschiller

codecov · 2022-09-29T05:54:23Z

Codecov Report

Merging #4371 (f761fae) into main (6fef2a1) will increase coverage by 0.01%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##             main    #4371      +/-   ##
==========================================
+ Coverage   49.73%   49.74%   +0.01%     
==========================================
  Files         899      899              
  Lines       26511    26518       +7     
  Branches     5407     5409       +2     
==========================================
+ Hits        13185    13192       +7     
  Misses      12404    12404              
  Partials      922      922

Impacted Files	Coverage Δ
src/utils/SimpleEventTarget.ts	`18.75% <ø> (ø)`
src/components/jsonTree/treeHooks.tsx	`72.22% <50.00%> (+1.63%)`	⬆️
src/contentScript/sidebarController.tsx	`10.65% <100.00%> (ø)`
src/runtime/pathHelpers.ts	`93.22% <100.00%> (+0.76%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

fregante · 2022-09-30T07:42:51Z

src/runtime/pathHelpers.test.ts

+
+  expectMatch(["user"], "user");
+  expectMatch(["users", 0], "users.0");
+  expectMatch(["users", 0, "name"], "users.0.name");


Note: lodash appears to suggest using a[0].b instead of a.0.b, but I have not done that because:

we're already using the latter

the latter is better looking

lodash.toPath supports both and has no difference in output, both produce exactly ["a", "0", "b"]

To be honest, I don't really like that we have a second form of array access like this. It makes me feel like there's something down the line like a tool or library or something where this will break. It feels like one of those things where we're arbitrarily breaking a common standard because it sorta works and things just kinda ended up that way. Is there a good reason we shouldn't keep things standardized to [0] for array access and property paths?

Is there a good reason we shouldn't keep things standardized to [0] for array access and property paths?

IIRC, it was a limitation with mustache not supporting bracket syntax, e.g., see janl/mustache.js#158

My preference would be for the runtime to support both syntax (dot notations works because arrays are objects...), but to standardize what syntax PixieBrix generates/suggests.

I agree that brackets is likely the way to go for arrays to be consistent with the syntax most people are familiar with. However, I would vote ["a", "0", "b"] should still generate a.0.b because the type of the "0" is a string. (I would avoid being fancy with implicit conversion)

So no change, right? ["a", "0", "b"] and ["a", 0, "b"] are the same since all keys/indexes are strings: [4,8,16]["1"] === 8

My two cents. A nested property path is not exactly the same as accessing a property in code. I prefer a.0.b when it comes to property path (frankly speaking I'd like to see something like a.0."User Location".b if it was supported). Reasons:

consistency. the only property separator is .. always (or as often as possible).

needless complication of logic. to build an accessor that is specific for an array (a[0] instead of a.0) we'll need to actually find out that the container is an array, but for the sake of getting a value of a deeply nested property it doesn't matter whether a container is an object or an array.

numbered properties can belong to objects. { 1: 'user'} is not an array. Formik's error tree is an example of such container, it's an object that may have integer keys. we can end up with paths like a.1.b and a[1].b.

👍

A couple of corrections though:

It never matters, arrays are objects: obj[1] is exactly the same as arr[1]

There's no such thing as integer keys. {1: 'str'} is casted to object with key '1' and value 'str'. Even array keys are effectively strings, try Object.keys([9,9,9])

twschiller · 2022-09-30T21:11:06Z

Adding @BLoe as reviewer since he had investigated a bit on Wednesday. DM thread: https://pixiebrix.slack.com/archives/D029R8902JF/p1664400635838989.

Potential issue:

BLoe · 2022-09-30T22:13:49Z

The issue is that we use the isVarValue() function in TextWidget to control some auto-switching of the field input toggle mode. I was able to get a regex working on regex101.com, but it wasn't quite working in the app itself, I'm not sure what the issue was, I stopped to move on to other things.

The problem here is:

If I paste in a copied path that includes a space, it will not auto-toggle to @ var input, and also it will auto-toggle back to text every time you change a character in the input at all, because it keeps detecting the space in the string
If I am typing out a variable path that includes a space, it will auto-toggle over to text as soon as I type a space, and then it will auto-toggle to text with every character typed after the space is there

twschiller · 2022-10-01T03:10:05Z

The problem here is:

I think we'll likely have to keep track of if there's an unclosed quotation mark? You'd have to write a function for that since IIRC regexes can't count

As a reminder, this only applies if the expression starts with a @, which simplifies the logic a bit

Does lodash have a validation library (or lexer/parser) we can use? Would be great to use the same logic that the expression interpreter is using

fregante · 2022-10-01T05:59:23Z

auto-toggle back to text every time you change a character in the input at all, because it keeps detecting the space in the string

I think that's an issue, UIs should not ignore or revert the user’s input. Our toggles (and underlying blueprints) should default to an "auto" mode that picks the right type until the user picks one.

It makes sense to update the type as the user types, but not at the expense of their explicit choice. Pasting @twschiller should be fine but incompatible with auto-toggling.

Does lodash have a validation library (or lexer/parser) we can use?

The logic is here, but _.get seems to accept any kind of strings without throwing.

https://github.com/lodash/lodash/blob/2da024c3b4f9947a48517639de7560457cd4ec6c/.internal/stringToPath.js#L5

As for this PR, it doesn't actually change the described behavior, which has always existed and should be tracked/fixed separately as it's a UI problem.

BALEHOK · 2022-10-03T15:17:36Z

src/runtime/pathHelpers.ts

+ * @example getPathFromArray("user", "name") // => "user.name"
+ * @example getPathFromArray("title", "Divine Comedy") // => "title["Divine Comedy"]"
+ */
+export function getPathFromArray(parts: Array<string | number>): string {


Can we reuse joinName with 'space' added to the specialCharsRegex?

It looks right except it uses compact and it seems specific to formik. While there's some overlap, the logic is too simple to add further conditions.

However potentially "getPathFromArray" could be used inside "joinName" after the compact call and error throw

BALEHOK · 2022-10-03T16:02:38Z

@fregante regarding joinPathParts

pixiebrix-extension/src/utils.ts

Lines 83 to 93 in 3cce78f

    
           /** 
        
            * Join parts of a path, ignoring null/blank parts. 
        
            * Works faster than joinName. 
        
            * Use this one when there're no special characters in the name parts or 
        
            * the parts contain already joined paths rather than individual property names 
        
            * @param nameParts the parts of the name 
        
            */ 
        
           export function joinPathParts(...nameParts: Array<string | number>): string { 
        
             // Don't use lodash.compact and lodash.isEmpty since they treat 0 as falsy 
        
             return nameParts.filter((x) => x != null && x !== "").join("."); 
        
           }

I think there's no need to change this function. It is meant to quickly concatenate string parts into a .-separated path. Normally it should be used only with our own, known in the dev time fields (or with already escaped paths) and never with user-generated inputs. It's the other function joinName that handles special chars in field names.

pixiebrix-extension/src/utils.ts

Line 57 in 3cce78f

export function joinName(

BALEHOK

This PR has been in review for quite a while. It seems that we're good with the update and no functionality is broken by the changes. If this is the case I believe we can merge it.

Add support for spaces in "Copy Property Path"

3cce78f

fregante added the bug Something isn't working label Sep 29, 2022

Update snapshots

f761fae

vercel bot deployed to Preview September 29, 2022 05:47 View deployment

fregante changed the title ~~Add support for spaces in "Copy Property Path"~~ #4367: Add support for spaces in "Copy Property Path" Sep 30, 2022

fregante self-assigned this Sep 30, 2022

fregante requested a review from twschiller September 30, 2022 07:38

fregante commented Sep 30, 2022

View reviewed changes

twschiller requested review from BLoe and BALEHOK September 30, 2022 21:09

BALEHOK reviewed Oct 3, 2022

View reviewed changes

BALEHOK approved these changes Oct 7, 2022

View reviewed changes

fregante merged commit f0cca2d into main Oct 7, 2022

fregante deleted the F/bug/support-spaces branch October 7, 2022 08:48

twschiller added this to the 1.7.10 milestone Oct 11, 2022

twschiller mentioned this pull request Oct 11, 2022

Page editor auto-complete bug when uses space #4438

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#4367: Add support for spaces in "Copy Property Path" #4371

#4367: Add support for spaces in "Copy Property Path" #4371

fregante commented Sep 29, 2022 •

edited

codecov bot commented Sep 29, 2022

fregante Sep 30, 2022

BLoe Sep 30, 2022 •

edited

twschiller Oct 1, 2022 •

edited

fregante Oct 1, 2022 •

edited

BALEHOK Oct 3, 2022 •

edited

fregante Oct 3, 2022 •

edited

twschiller commented Sep 30, 2022

BLoe commented Sep 30, 2022

twschiller commented Oct 1, 2022 •

edited

fregante commented Oct 1, 2022 •

edited

BALEHOK Oct 3, 2022 •

edited

fregante Oct 3, 2022

BALEHOK commented Oct 3, 2022 •

edited

BALEHOK left a comment

	export function joinPathParts(...nameParts: Array<string \| number>): string {
	// Don't use lodash.compact and lodash.isEmpty since they treat 0 as falsy
	return nameParts.filter((x) => x != null && x !== "").join(".");
	}

#4367: Add support for spaces in "Copy Property Path" #4371

#4367: Add support for spaces in "Copy Property Path" #4371

Conversation

fregante commented Sep 29, 2022 • edited

What does this PR do?

Discussion

Demo

Future work

Checklist

codecov bot commented Sep 29, 2022

Codecov Report

fregante Sep 30, 2022

Choose a reason for hiding this comment

BLoe Sep 30, 2022 • edited

Choose a reason for hiding this comment

twschiller Oct 1, 2022 • edited

Choose a reason for hiding this comment

fregante Oct 1, 2022 • edited

Choose a reason for hiding this comment

BALEHOK Oct 3, 2022 • edited

Choose a reason for hiding this comment

fregante Oct 3, 2022 • edited

Choose a reason for hiding this comment

twschiller commented Sep 30, 2022

BLoe commented Sep 30, 2022

twschiller commented Oct 1, 2022 • edited

fregante commented Oct 1, 2022 • edited

BALEHOK Oct 3, 2022 • edited

Choose a reason for hiding this comment

fregante Oct 3, 2022

Choose a reason for hiding this comment

BALEHOK commented Oct 3, 2022 • edited

BALEHOK left a comment

Choose a reason for hiding this comment

fregante commented Sep 29, 2022 •

edited

BLoe Sep 30, 2022 •

edited

twschiller Oct 1, 2022 •

edited

fregante Oct 1, 2022 •

edited

BALEHOK Oct 3, 2022 •

edited

fregante Oct 3, 2022 •

edited

twschiller commented Oct 1, 2022 •

edited

fregante commented Oct 1, 2022 •

edited

BALEHOK Oct 3, 2022 •

edited

BALEHOK commented Oct 3, 2022 •

edited