Skip to content
This repository has been archived by the owner on Aug 4, 2021. It is now read-only.

Add RegExp support and strict order of entries (+unit-tests) #53

Merged
merged 19 commits into from
Aug 22, 2019

Conversation

thiscantbeserious
Copy link
Contributor

@thiscantbeserious thiscantbeserious commented Aug 1, 2019

Added support for Regular Expressions and switched from an object to an array for the entries to keep a strict order in which rules are queried (unlike previously).

This will change the options a bit - with the added bonus of better readability (basically you'll have to specify the 'find' property instead of abusing the object-key for the search-string.

Edit: Maybe a Set would give better performance, will have to revisit later on - for now this is sufficient to get started (at least for us)

Edit 2: Also keep in mind that this is currently not backwards compatible with the old configuration, that would need some extra work, if desired.

Edit 3: For examples see README.md in my Repo.

@thiscantbeserious
Copy link
Contributor Author

thiscantbeserious commented Aug 1, 2019

Hurr, pardon me for the weird commit comments in regards to the README.md 🐵

(ffs, almost typed .me again).

@thiscantbeserious
Copy link
Contributor Author

thiscantbeserious commented Aug 16, 2019

Ping @shellscape maybe that PR is interesting for you guys? Otherwise I'll release this as a fork (I'd hate doing that, out of principle).

@shellscape
Copy link
Contributor

@thiscantbeserious please bear with us. We're a team of 3 (sometimes 4) right now juggling the ecosystem 😄 We just got about 2/3 of the way through the rollup core backlog and the plugins are next on my list. I promise you we'll get to this soon.

@thiscantbeserious
Copy link
Contributor Author

thiscantbeserious commented Aug 20, 2019

I'm fine with that 💃 ...

I actually forgot to mention that this also supports subpattern matching & replacing -

so this can be very powerfull in total, e.g. when migrating a larger code-base with previous loader-syntax support - for example to remove those to get rollup to correctly load the files:

{find:/^i18n\!(.*)/, replacement:"$1.js" }

I'm not to good with examples so maybe when you merge it (if you do) maybe you'll come up with better examples or a better Readme to outline the changes 🔢 .

Cheers!

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work, I think this is a really nice improvement, though of course it is a breaking change. Left some comments and suggestions.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
test/index.js Outdated Show resolved Hide resolved
Copy link
Contributor Author

@thiscantbeserious thiscantbeserious left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleaned the mess up and pushed it to my repo.

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, this looks good from my side!

@@ -27,36 +27,31 @@ For Webpack users: This is a plugin to mimic the `resolve.alias` functionality i
```
$ npm install rollup-plugin-alias
```

#
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this intended?

};
```
If not given local aliases will be resolved with a `.js` extension.
You can use either simple Strings or Regular Expressions to search in a more distinct and complex manner (e.g. to do partial replacements via subpattern-matching, see aboves example).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

@thgh
Copy link

thgh commented Sep 17, 2019

Too bad this was not implemented with backwards compatibility. Objects have a strict ordering as far as I know https://www.stefanjudis.com/today-i-learned/property-order-is-predictable-in-javascript-objects-since-es2015/

It seems that only a tiny fraction of projects needs the regex feature while other projects now get a more complex configuration. If you consider accepting a PR that restores backwards compatibility I would be willing to try that.

This seems not to be true anymore:
For Webpack users: This is a plugin to mimic the resolve.alias functionality in Rollup.

@shellscape
Copy link
Contributor

@thgh that sounds like a regression if back-compat has been lost. Would you be willing to open a new issue (or straight to PR if you'd like). I'm absolutely certain we'd entertain restoring back-compat.

@thiscantbeserious
Copy link
Contributor Author

thiscantbeserious commented Sep 17, 2019

Too bad this was not implemented with backwards compatibility. Objects have a strict ordering as far as I know https://www.stefanjudis.com/today-i-learned/property-order-is-predictable-in-javascript-objects-since-es2015/

It seems that only a tiny fraction of projects needs the regex feature while other projects now get a more complex configuration. If you consider accepting a PR that restores backwards compatibility I would be willing to try that.

Actually you don't need to use Regular Expressions they are still
completely optional.

I can understand that you might feel a bit confused by the differences in
configuration at first but there are technical reasons for the change in
regards to it as well.

Previously in V1 it was abusing object-properties (key = find, value =
replacement) as entries like you would fill an array.

While an object key can actually hold any string it can only be accessed by
the square brackets accessor - that in itself wouldn't be that bad if it
wasn't for the sorting order (besides that it could be considered bad
practice in terms of clean code).

Let me give you an example (not syntax correct I know - just for the sake of keeping things together):

{
"my/super/awesome/test/import": "redirect/to/my/face",
"awesome":"bad"
}

Logically speaking (what the user excepts) when using the above config all
imports of my/super/awesome/test/import would first be replaced with
redirect/to/my/face and then all occurances of awesome would be replaced
with bad.

Right?

That's not what happens in V1.

Objects sort properties alphabetically by their key (you can verify that by
copying the above example to your console and checking the resulting object
value).

You'll get this:

So what would happen is that the rule "awesome" would be executed first,
replacing all occurances of awesome first - as such rewriting all imports
of my/super/awesome/test/import to my/super/bad/test/import - and your
first rule would actually never apply at all.

Now that is just an super simple example - imagine like multiple rules
working like this together.

This seems not to be true anymore:
For Webpack users: This is a plugin to mimic the resolve.alias functionality in Rollup.

I think we can agree on that its a bad reference and never actually worked that way anyway (as such I should be simply removed - I mean that reference)?

Imagine them completely changing the way resolve.alias in the next version - just for the sake of it should we adjust? I don't think so. A plugin should fullfill a specific purpose - and use the configuration it needs and not what someone else decides it should have just for the sake of it

@shellscape I would agree on simplifying things but full backwards compability is not 100% fesible as I see it - not in regards to using object keys for the definition see my above explanation.

@lukastaegert actually agreed on the breaking changes by definition - I mentioned that in the PR - so it didn't accidently slipped in as far as I could get.

This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants