search

ljharb / js-traverse

MIT License
45 stars 8 forks source link

Symbol support breaking change #5

Closed MartyJRE closed 4 months ago

MartyJRE commented 6 months ago

Would you consider changing the symbol support from 7d659e78d43e69e6afc604e12b3ba3a5def0e46f to an optional feature enabled by a flag of some kind?

I get the improvement this brings, however it creates a MAJOR change for libraries and projects that depend on traverse.

For example older versions of Loopback 4 use babel/traverse which depends on traverse. They specify any MINORs or PATCHes as OK with the ^ symbol in their package.json file. Since this feature wasn't released as a new MAJOR version, they may start using it. The interface never contained anything other than strings, so this breaks when iterating over data with symbols.

ljharb commented 6 months ago

Can you elaborate? Enumerable own symbols are rare; in what concrete case is this causing breakage for someone?

MartyJRE commented 5 months ago

Loopback depends on traverse. They use it to walk REST connector objects. This behavior is fine while all leaves of the object are non-symbol. However, nothing's stopping you from setting one of the leaves to another complex object. In our project, which depends on both loopback and traverse, we include an instance of forever-agent in the tree. Forever agent likely contains request, response and other complex objects. These contain symbols. Thus the new feature breaks dependant libraries who were expecting the interface to be string up until now.

MartyJRE commented 5 months ago

https://github.com/loopbackio/loopback-connector-rest/blob/3d0ea7b8ff59050796c09a8d037eefc8835d0658/lib/template.js#L52

In this file, loopback-rest-connector expects the output of traverse to be string. They call RegExp.exec() on the item, which can now be a symbol. This crashes.

ljharb commented 5 months ago

That is… impressively convoluted, and I think technically and unfortunately makes it qualify as an actual breaking change.

likely contains request, response and other complex objects. These contain symbols.

can you confirm that they do, in fact, contain enumerable symbols?

If it's just theoretical, then I don't necessarily need to change anything, but if existing non-contrived code that worked before, breaks now, then I do.

MartyJRE commented 5 months ago

This screenshot was taken in the debugger during the operation I described:

image

It seems the agent doesn't contain a request and response as I thought. However, it does contain its own symbols.

MartyJRE commented 5 months ago

I filed an issue with loopback as well, but I feel like this needs to be fixed in both places.