feat: add codemods for es-*
#52
Conversation
feat: add `es-set-tostringtag`
| return dirtyFlag ? root.toSource(options) : file.source; | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
es-set-tostringtag sure should be replaced, but it's a little more complex than this logic (third argument and existence check). In most cases, it will work fine, but I remember that there were some potentially dangerous cases.
There was a problem hiding this comment.
I used the example from the es-set-tostringtag docs. Do you know where else I can find examples with more complex logic?
There was a problem hiding this comment.
I'm not sure what is the correct way to deal with it. What should the output code look like?
Should I add a condition before calling Object.defineProperty like this if force isn't specified?
!Object.hasOwn(tagged, Symbol.toStringTag) &&
Object.defineProperty(tagged, Symbol.toStringTag, {
configurable: true,
value: "new tag",
});
What about If I can't determine the value of force?
Should I replace it with a function that handles these cases?
There was a problem hiding this comment.
What about If I can't determine the value of
force?
If the third arg passed to this function - (force || !Object.hasOwn(...)) && ..., if not - just !Object.hasOwn(...) && ....
There was a problem hiding this comment.
I was able to handle the case where force is set to true but I wasn't able to handle more complex cases such as
const force = true
setToStringTag(obj, 'tagged', { force });or
const options = { force: true }
setToStringTag(obj, 'tagged', options);
There was a problem hiding this comment.
It could be worth doing a github search for that library and seeing the usage. It can give an insight into how many people are using it in a certain way. If not many people are using force in the first place, we can leave it for now and try to improve on it later. With codemods there will always be edgecases, not everything is very easy to statically analyze, but if we can get 90% of the usecases covered, and log a warning for cases it wasnt able to analyze/transform, I think thats fine.
There was a problem hiding this comment.
Yeah, a quick search on https://github.com/search?q=require%28%27es-set-tostringtag+language%3AJavaScript&type=code makes it seem like the force isn't being used that much, so we can leave it for now
There was a problem hiding this comment.
Ok, would you like me to change anything else?
There was a problem hiding this comment.
I think this LGTM for now. Let's improve the codemods more when we do some battletesting with them on real world projects :)
Add the following codemods
es-get-iteratores-set-tostringtages-define-propertyI'm not too familiar with
jscodeshiftso I'm not sure if there are problems with my codemods.Also, I had some type errors. I'm not sure if this is expected or if it can be fixed