fix: pattern match of topLevelImportPaths was unanchored

[agriffis]

This seems to have been a mistake in #354

It might fix https://github.com/styled-components/styled-components/issues/3635 but that's somewhat involved to test while this is unreleased, so it might be easier to make this release and find out.

[rockwotj]

It's unclear to me that this causes breakage, the option is undocumented and normally the empty array, so how that's causing breakage is confusing to me. Anyways, anchoring this regex should not be forced here in my opinion. And the built-in matches are bounded.

The test change looks good though

[agriffis]

@rockwotj It's rarely empty because the most common way to use the plugin is through the macro

https://github.com/styled-components/styled-components/blob/30dab74acedfd26d227eebccdcd18c92a1b3bd9b/packages/styled-components/src/macro/index.ts#L41

[agriffis]

Anyways, anchoring this regex should not be forced here in my opinion. And the built-in matches are bounded.

The point is to preserve some semblance of backward compat. Bare strings will probably work as expected so long as they're anchored. For advanced cases, you can easily prepend or append .*

[rockwotj]

That's fair, note the opinion name changed so that macro code doesn't work either.

EDIT: ah nevermind it's not renamed.

[rockwotj]

Anyways since this is also a "breaking" change, should we just switch to picomatch?

[rockwotj]

I'm still a bit confused as to why the macro broke. The default pattern is here which is the same as the default packages that get checked with a bounded regex here

[agriffis]

Anyways since this is also a "breaking" change, should we just switch to picomatch?

I agree it's a breaking change at this point. I really liked your suggestion to use picomatch earlier—I think it could have prevented the original breaking change, because it's extremely unlikely that any ordinary module strings would contain picomatch globbing chars. I don't have a strong feeling about using picomatch now, except to avoid this getting stuck in a cycle of well-meaning "should we just" alternatives. :grimacing:

Anchoring is near at hand, but if you want to rework it to use picomatch, I'd support that.

I'm still a bit confused as to why the macro broke.

Unfortunately, I don't think this is the only problem involved. Between styled-components, this plugin, and the macro, there are a lot of issues open at the moment, with various claims that pinning one thing or the other solves it. Unfortunately, working on this amounts to yak shaving for most of us, myself included. I'm trying to gradually work through them, though.

[rockwotj]

Ah so I tried and picomatch was not able to support "going up" an arbitrary number of directories, which is often the usecase for relative imports (../../../../resources/my-local-styled-components) which was my usecase. I don't think picomatch will work for that.

FWIW I tried out the tests in the styled-components repo for the macro and they pass with the latest version, so I'm not sure what the difference is. I did send a PR to fix this in the macro, but if we merge this we'll probably want to modify that PR to remove the bounds.

[agriffis]

Not advocating for picomatch, but this works:

> const pm = require('picomatch')
> pm('*(../)*.js')('../../../foo.js')
true

See https://github.com/micromatch/picomatch#extglobs

If we still want to consider alternatives, what about your other suggestion to use objects? It could be:

{
  "topLevelImportPaths": [
    {"match": "/my-local-styled-components$"}
  ]
}

Existing strings would be used as literals, so you'd opt into the regex syntax explicitly with the object format.

Frankly, I would probably prefer that over the approach in this PR, but I'd take either one! :joy:

[rockwotj]

Ah good find with the extended glob patterns. I've opened a PR to use picomatch: https://github.com/styled-components/babel-plugin-styled-components/pull/369

[agriffis]

Closing in favor of #369