1. 4
    Let us talk about JSON.stringify() javascript pulltech.net
  1.  

  2. 24

    Please don’t suggest running a string-replace regex on JSON. That’s horribly dangerous.

    1. 4

      Sometimes, needs must as the devil drives.

      (Watched this approach blow up at work because it would be easier than using jq. Well, guess how that ended…)

      1. 2

        Yeah this is definitely a case of “don’t do what Donnie Dont does”

      2. 13

        There is quite a bit wrong with the approach recommended in this article.

        Example:

        JSON.stringify({ foo: “not an _id”, _id:“It’san _id” }).replace(/_id/gi, “id”)

        {"foo":"not an id","id":"It'san id"}

        And this is just a taste of the potential badness here iceberg.

        1. 5

          Using serialization and de-serialization to mungle object keys is a horrible idea! Not only will it be much slower, and allocate much more than a structured-clone, it also introduces all these value rules you need to keep in mind!

          I’d much rather see a recursive clone solution than JSON-and-Regexp.

          1. 5

            I’d say that for a case this small, it’d be far easier to just return a new object, like so:

            const todayILearnSource = {
              _id: 1,
              content: '今天学习 JSON.stringify(),我很开心!',
              created_at: 'Mon Nov 25 2019 14:03:55 GMT+0800 (中国标准时间)',
              updated_at: 'Mon Nov 25 2019 16:03:55 GMT+0800 (中国标准时间)'
            };
            
            const todayILearn = {
              id: todayILearnSource._id,
              content: todayILearnSource.content,
              createdAt: todayILearnSource.created_at,
              updatedAt: todayILearnSource.updated_at
            }
            

            Obviously, there’s a limit to how much it scales, but there’s no loops, you can use that expression as a return statement, etc. For something this simple, it’s probably better to stay as simple as possible, unless you’re only touching a minority of the keys in an object, in which case one of the other suggested things in the comments here might work well. KISS

            1. 4

              Nice write up! I do understand stringify better now :)

              In the first example, I would agree with @pilif that the regex is a bit of a dangerous solution. I think I’d go for something like this:

              const mapObj = {
                  _id: "id",
                  created_at: "createdAt",
                  updated_at: "updatedAt",
              };
              
              Object.entries(todayILearn).reduce((obj, [key, value]) => {
                  key = mapObj[key] || key;
                  return {
                      ...obj,
                      [key]: value,
                  };
              }, {});
              
              
              1. 2

                Nice functional solution. I write a lot of JavaScript at the moment that needs to be fast and understandable by people without too much JS experience, so my take would be something like this:

                function replaceKeys(keyMap, obj) {
                  const output = {};
                
                  for (let key in obj) {
                    const newKey = keyMap[key] || key;
                
                    output[newKey] = obj[key];
                  }
                
                  return output;
                }
                

                This wouldn’t handle every case, off the top of my head it wouldn’t handle symbols or a key of zero, but for the author’s usecase it would work fine.

              2. 3

                While this is not a good idea, I appreciate that someone made the effort to write it down and share it. It’s likely that it’s more than just the author who learned that this is not a good idea from the discussion the post catalysed, and so for that I hope we can refrain from being mean, or from flagging it as spam — which it certainly isn’t. I may have been wrong about this 😕

                1. 2

                  The ‘spam’ flags might not be related to this particular story: >75% of this user’s submissions are their own posts similar to this, with a total of 0 comments on lobsters. It can give the impression that the author here only uses lobsters to promote their blog since they don’t even contribute to the discussion on their own content.

                  1. 2

                    This link

                    https://www.pulltech.net/article/1582362508-Let-s-talk-about-JSON-stringify%28%29

                    redirects to

                    https://www.pixelstech.net/article/1582362508-Let-s-talk-about-JSON-stringify%28%29

                    which is the domain where the majority of the submissions by @pxlet are from: https://lobste.rs/newest/pxlet

                    It’s quite obvious to me that @pxlet bumped up against the number of submissions to one domain from one user recently implemented, and is using a new domain + a redirect to get around this.

                    Ping @pushcx for review.

                    1. 3

                      Yep. They bumped the limit trying to submit this story under the pixelstech.net domain and then submitted this a minute later. Looks like the domain name and redirect have been around a while. Looking at the profile, they’re pretty clearly here to promote their blog - 27 of 41 stories have been to it, no comments, and I can see they’ve done no voting.

                      I opened my laptop this morning to back that code out, though. As discussed, it has too many false positives. I’d like to post a meta thread about a comment from @zem I see as best capturing what bothers everyone most about this kind of behavior, but the clock is ticking towards the start of my workday.

                2. 3

                  I see 8 rules I need to keep in mind when using stringify() to map objects like this. Also, if multiple if/else statements are “not scalable at all”, how is the mapObj and the duplicated key strings in the regex better?

                  If I needed to do this I think I would reach for Map() instead of JSON.stringify(), but perhaps I’m missing something?

                  1. 18

                    Also, the regex replacement is dangerous as the keys might appear in the values of a different object with the same shape. This really isn’t a good solution.

                    1. 2

                      How does Map help?

                      1. 1

                        It doesn’t really; my kneejerk way of doing this would look like:

                        for (let [key, value] of new Map(Object.entries(todayILearn))) {
                            if (key === '_id') output.set('id', value);
                            else if (key === 'created_at') output.set('createdAt', value);
                            else if (key === 'updated_at') output.set('updatedAt', value);
                            else output.set(key, value);
                        }
                        

                        Now I’ve written it out it’s no different from the author’s first “inelegant” solution :-)

                        1. 1

                          It may seem a bit “ineleganter” because of these ifs and all, but looks significantly safer and significantly more readable to me then the stringify version there. I haven’t tested it, but I assume it’s also much more performant.

                          I think there is value in the article in that that it shows some “rules” on how things get stringified (I just wish the naming was better), but the example is a totally wrong one in my opinion - both in proposing the dangerous regex and the use case not being a good one for stringify in the first place, at least in my opinion.

                          Maybe with some work the article can get improved?

                    2. 2

                      Is this a joke? It’s mildly humorous as a joke and a straight up on-the-spot firing reason as a serious suggestion.

                      1. 1

                        While You Might Not Need lodash, it comes in handy for stuff like this. Use mapKeys and be done with it.

                        1. 1

                          Is this serious? Can code like this pass PR review?