Skip to content

Conversation

@rockwotj
Copy link
Contributor

This allows for local imports like ../../resources/styled where the relative path changes so the existing pattern doesn't work.

Tyler Rockwood added 2 commits September 13, 2021 16:14
This allows for local imports like ../../resources/styled where the relative path changes so the existing pattern doesn't work.
@quantizor
Copy link
Collaborator

@rockwotj can the existing solution support both path types?

@rockwotj
Copy link
Contributor Author

@probablyup not without breaking the format I think? How else can the string be interpreted as a regex or a literal? Happy to change the existing config option to be an array of objects with a 'type'.

We could also remove the existing config option, regexes are purely more powerful. As a compromise we could also use a library like picomatch to strike a middle ground and then it's also be unlikely we'd break existing patterns that way.

@rockwotj rockwotj changed the title Add topLevelPathPatterns option Change topLevelPaths option to take a regexp pattern Oct 19, 2021
@rockwotj
Copy link
Contributor Author

Okay @probablyup I've changed this so that the existing pattern now takes a regex. This is a breaking change for anyone that uses the (undocumented) topLevelImportPaths configuration.

@quantizor quantizor merged commit fa65030 into styled-components:main Nov 21, 2021
@agriffis
Copy link
Contributor

This change resulted in some fallout, see styled-components/styled-components#3635

Regexes are more powerful, but more confusing and prone to error. Either of the other suggestions (picomatch or typed objs) would have been a better approach, and would have avoided the breaking change.

As an example of prone to error, this PR uses re.test which isn't anchored, so now strings in topLevelImportPaths get converted to patterns that match anywhere in the input. The modified test even demonstrates the potential for mistakes:

       "@xstyled/styled-components/*"

This pattern uses * instead of .* but it matches '@xstyled/styled-components/test' anyway because re.test isn't anchored.

@rockwotj @probablyup It seems clear that pinning styled-components to version 1 of babel-plugin-styled-components isn't the best fix, even if it's a temporary workaround for the linked issue. Do you have thoughts on the best path forward?

agriffis added a commit to agriffis/babel-plugin-styled-components that referenced this pull request Dec 29, 2021
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.

3 participants