[PDE-683] (BREAKING) Resolve curlies to their original data type #139

stevelikesmusic · 2019-02-24T03:11:46Z

Description

CLI integrations always use bundle values directly from the bundle we inject to their request functions:

const perform = (z, bundle) => {
  const options = {
    url: 'http://api.example.com/post', 
	method: 'POST',
    body: {
      // We directly access the variable -- names: ['Ron', 'Veronica', 'Brick']
      names: bundle.inputData.names,
    }
  }

  z.request(options);
}

But, if we are using curly tokens to be replaced with an actual value, core interpolates value in a string. One example is in a perform shorthand RequestSchema.

const perform = {
  url: 'http://api.example.com/post',
  method: 'POST',
  body: {
    // String.prototype.replace will coerce the data type to a string
    // Using the case above -- names: 'Ron,Veronica,Brick' 
    names: '{{bundle.inputData.names}}',
  }

This fix keeps the data types of values replaced via curlies. The one exception is when we have an interpolated value as part of a string (ex. 'Bearer {{bundle.authData.access_token}}'). In this case we:

Add to the string as we did before
Throw an error if the type is a plain object or array

The tests do a better job of illustrating what we're doing now.

Jira

https://zapierorg.atlassian.net/browse/PDE-683

stevelikesmusic · 2019-02-24T03:17:37Z

src/tools/cleaner.js

-      const s = String(key).replace(/[-[\]/{}()\\*+?.^$|]/g, '\\$&');
-      const re = new RegExp(s, 'g');
-      out = out.replace(re, bank[key]);
+      const escapedKey = String(key).replace(/[-[\]/{}()\\*+?.^$|]/g, '\\$&');


Changed the names here to be a bit more descriptive. I think we could drop the String constructor. Since key is derived from Object.keys, we should be able to safely assume it is always a string.

I think you're right. Let's drop it!

stevelikesmusic · 2019-02-24T03:25:22Z

src/tools/data.js

@@ -14,6 +14,18 @@ const isPlainObj = o => {

 const comparison = (obj, needle) => obj === needle;

+const getObjectType = obj => {
+  if (_.isPlainObject(obj)) {


Any reason we have our own isPlainObject? Is it preferred to lodash's?

🤷‍♂️ but yeah, I think we should stick to lodash's.

stevelikesmusic · 2019-02-24T03:25:57Z

src/tools/data.js

@@ -130,8 +142,7 @@ const recurseExtract = (obj, matcher) => {
 const _IGNORE = {};

 // Flatten a nested object.
-const flattenPaths = (data, sep) => {
-  sep = sep || '.';
+const flattenPaths = (data, sep = '.') => {


eliangcs

Tested locally, I don't find any issues. Good work!

There's a failing test you need to fix in test/logger.js. Also got some minor suggestions but overall it's a 👍from me. Since it's a breaking change, maybe @bcooksey would like to take a quick look?

eliangcs · 2019-02-25T07:52:16Z

src/tools/cleaner.js

-      const s = String(key).replace(/[-[\]/{}()\\*+?.^$|]/g, '\\$&');
-      const re = new RegExp(s, 'g');
-      out = out.replace(re, bank[key]);
+      const escapedKey = String(key).replace(/[-[\]/{}()\\*+?.^$|]/g, '\\$&');


I think you're right. Let's drop it!

eliangcs · 2019-02-25T08:04:21Z

src/tools/cleaner.js

+      const isPartOfString = valueParts.length > 1;
+
+      if (isPartOfString) {
+        if (_.isArray(replacementValue) || _.isPlainObject(replacementValue)) {


I think here we can use native Array.isArray instead of lodash's _.isArray.

Totally agree. Only used _.isArray since we were grabbing _.isPlainObject right next to it. No strong feeling about it though

eliangcs · 2019-02-25T08:13:49Z

src/tools/cleaner.js

+      }
+
+      out = isPartOfString
+        ? valueParts.join('').replace(matchesKey, replacementValue)


Looks like valueParts.join('') always equals to out here. So can we simplify this line to the following?

Suggested change

? valueParts.join('').replace(matchesKey, replacementValue)

? out.replace(matchesKey, replacementValue)

Doh. Nice catch @eliangcs

eliangcs · 2019-02-25T08:14:48Z

src/tools/data.js

+    return 'Object';
+  }
+
+  if (_.isArray(obj)) {


Likewise, native Array.isArray is preferred.

Yep, same reason as above. Also don't feel strong about it here either.

eliangcs · 2019-02-25T08:17:56Z

src/tools/cleaner.js

-const flattenPaths = require('./data').flattenPaths;
+const { isPlainObj, getObjectType } = require('./data');
+const { recurseReplace } = require('./data');
+const { flattenPaths } = require('./data');


Nice cleanup! Can we make it even better by importing them in one statement?

const { isPlainObj, getObjectType, recurseReplace, flattenPaths } = require('./data');

Haha yikes. I just now noticed that those were coming from the same file. Thanks for that!

xavdid

I agree with CH's great review (as per usual). Added a minor gripe on the error message text, but otherwise 👍

xavdid · 2019-02-25T17:16:39Z

src/tools/cleaner.js

+              'Cannot interpolate objects or arrays into a string.',
+              `You've sent an ${getObjectType(replacementValue)},`,
+              `which will get coerced to ${replacementValue}`
+            ].join(' ')


is there a reason this is an array vs a string that prettier breaks up automatically?

Yeah, I just found it easier to read this way than the way that prettier was formatting it. I don't feel strongly about, though. Can totally be just a string.

xavdid · 2019-02-25T17:19:34Z

test/create-request-client.js

+          message: 'No arrays, thank you: {{bundle.inputData.badData}}'
+        }
+      }).should.be.rejectedWith(
+        "Cannot interpolate objects or arrays into a string. You've sent an Array, which will get coerced to 1,2,3"


coerced to 1,2,3

I find this wording a little confusing, since it's not clear that 1,2,3 is a string. Maybe change the error to be

which will get coerced into the string "1,2,3"

also maybe change it to Cannot reliably interpolate... to make it clear that we can, but it might have weird behavior.

Good points

Is which will get coerced into the string actually true? We are always going to throw this error, yea? Maybe just a stringified dump of the first 10-20 characters to help them know which array we are getting?

Yeah it will always throw. The intent was to say, "Hey there, here's what would have gone through". But maybe that's unnecessary. We could just say, "Can't do it. Here's what you sent".

We are still sending them the data above. But I can see the value in keeping the output simpler.

bcooksey · 2019-02-25T18:38:01Z

test/create-request-client.js

+          message: 'No arrays, thank you: {{bundle.inputData.badData}}'
+        }
+      }).should.be.rejectedWith(
+        "Cannot interpolate objects or arrays into a string. You've sent an Array, which will get coerced to 1,2,3"


Is which will get coerced into the string actually true? We are always going to throw this error, yea? Maybe just a stringified dump of the first 10-20 characters to help them know which array we are getting?

stevelikesmusic added the enhancement label Feb 24, 2019

stevelikesmusic requested review from eliangcs and xavdid February 24, 2019 03:11

stevelikesmusic force-pushed the fix/curly-data-types branch from c2810de to d21cdee Compare February 24, 2019 03:15

stevelikesmusic commented Feb 24, 2019

View reviewed changes

stevelikesmusic force-pushed the fix/curly-data-types branch from d21cdee to 6665bdd Compare February 24, 2019 03:23

stevelikesmusic commented Feb 24, 2019

View reviewed changes

eliangcs approved these changes Feb 25, 2019

View reviewed changes

eliangcs changed the title ~~[PDE-683] Resolve curlies to their original data type~~ [PDE-683] (BREAKING) Resolve curlies to their original data type Feb 25, 2019

xavdid approved these changes Feb 25, 2019

View reviewed changes

bcooksey approved these changes Feb 25, 2019

View reviewed changes

stevelikesmusic force-pushed the fix/curly-data-types branch from 931d132 to 98a07f5 Compare February 25, 2019 19:47

[PDE-683] Resolve curlies to their original data type

53472e2

stevelikesmusic force-pushed the fix/curly-data-types branch from 98a07f5 to 53472e2 Compare February 25, 2019 19:56

stevelikesmusic merged commit f42f36a into master Feb 25, 2019

stevelikesmusic deleted the fix/curly-data-types branch February 25, 2019 20:01

eliangcs mentioned this pull request Apr 7, 2020

feat(core,schema,legacy-scripting-runner): allow to "reliably interpolate arrays or objects to a string" zapier/zapier-platform#190

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PDE-683] (BREAKING) Resolve curlies to their original data type #139

[PDE-683] (BREAKING) Resolve curlies to their original data type #139

stevelikesmusic commented Feb 24, 2019

stevelikesmusic Feb 24, 2019

eliangcs Feb 25, 2019

stevelikesmusic Feb 24, 2019

eliangcs Feb 25, 2019

stevelikesmusic Feb 24, 2019

eliangcs left a comment •

edited

Loading

eliangcs Feb 25, 2019

eliangcs Feb 25, 2019

stevelikesmusic Feb 25, 2019

eliangcs Feb 25, 2019

stevelikesmusic Feb 25, 2019

eliangcs Feb 25, 2019

stevelikesmusic Feb 25, 2019

eliangcs Feb 25, 2019

stevelikesmusic Feb 25, 2019

xavdid left a comment

xavdid Feb 25, 2019

stevelikesmusic Feb 25, 2019

xavdid Feb 25, 2019

xavdid Feb 25, 2019

stevelikesmusic Feb 25, 2019

bcooksey Feb 25, 2019

stevelikesmusic Feb 25, 2019

bcooksey Feb 25, 2019

	? valueParts.join('').replace(matchesKey, replacementValue)
	? out.replace(matchesKey, replacementValue)

[PDE-683] (BREAKING) Resolve curlies to their original data type #139

[PDE-683] (BREAKING) Resolve curlies to their original data type #139

Conversation

stevelikesmusic commented Feb 24, 2019

Description

Jira

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

eliangcs left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xavdid left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliangcs left a comment •

edited

Loading