Skip to content

Conversation

@tomas-ki
Copy link

@tomas-ki tomas-ki commented Jun 27, 2023

This allows preserve option, which will preserve specified characters both when using strict mode and not.
It allows for different structures, like test_property and test.property being valid. Another example is having more complex paths: path.to.property_name.

I think this eloquently fixes #131 as well.

Does not affect current implementation in any way so shouldn't be a breaking change.

@Trott
Copy link
Collaborator

Trott commented Jun 27, 2023

As implemented here, there's nothing enforcing that the elements of preserve are a single character, so lots of things can be passed that will result in weird bugs because they won't be escaped properly or whatever. This is especially true given that an option like this is bound to be used for special regex characters like [ and ]. Would it make sense to throw an error if the user passes an array containing strings of length greater than one? Or maybe it makes sense to have the user pass in a single string containing all the characters to be preserved rather than an array of single characters? Then you can split() it into an array of single-character strings? There might be surprising bugs still lurking in such an approach, but it seems less of a footgun for users than the current implementation.

It's all but certain that someone will pass ['/%*'] rather than ['/', '%', '*'] and I think we should throw an error if someone does that (either because each element should have a length of 1 or because we write the API to expect a string and not an array of characters).

@Trott
Copy link
Collaborator

Trott commented Jun 27, 2023

As an example of some weirdness that we'll want to guard against:

  1. Using preserve: [''] results in a SyntaxError. We should provide an error telling the person that they can't preserve an empty string or something like that.
  2. Using preserve: ['.]'] preserves /, _, etc. That will be a very surprising result to whoever runs across it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants