Refactor for better compatibility and speed. #52
Conversation
Real apps are way bigger than the example, so this will be more useful
.gitignore
Outdated
| @@ -1,3 +1,4 @@ | |||
| .vscode | |||
There was a problem hiding this comment.
I would suggest putting this in your system gitignore file.
cibernox
force-pushed
the
specialised-regexes-per-type
branch
from
2393e36
to
84decda
Compare
|
Need to change https://github.com/rickharrison/broccoli-asset-rewrite/blob/master/.travis.yml#L3 to "4" to allow the tests to run... |
|
Happy to chat. Or if one of the ember people wants to take charge on this, that is cool as well. Let me know how I can help. |
tests/filter-tests.js
Outdated
| @@ -148,6 +148,7 @@ describe('broccoli-asset-rev', function() { | |||
| var node = new AssetRewrite(sourcePath + '/input', { | |||
| replaceExtensions: ['js'], | |||
| assetMap: { | |||
| 'images/some-image.jpg': 'images/some-image-1a2b3c4e.jpg', | |||
| 'the.map' : 'the-other-map', | |||
| 'http://absolute.com/source.map' : 'http://cdn.absolute.com/other-map' | |||
There was a problem hiding this comment.
@rickharrison does this line make sense in the real world? Is it a real situation where you have an absolute sourcemap and you want to replace by a different absolute sourcemap? This is the only failing test (since the prepend is set, my algorithm generates https://cloudfront.net/http://cdn.absolute.com/other-map), but I don't see much sense on this operation at all.
There was a problem hiding this comment.
That is from https://github.com/rickharrison/broccoli-asset-rewrite/pull/8/files. I am not that familiar with source maps, but I'm sure there is a reason it was included as a test case.
There was a problem hiding this comment.
@ef4 maybe you can cast some light over this? I'd expect this library to replace paths while optionally prepending hosts, but not replace one absolute path by another.
There was a problem hiding this comment.
@cibernox There's no strong convention for how sourcemaps should be named and located, so I was being as generic as possible. But I don't think this is a critical feature and I'm 👍 with changing that test.
There was a problem hiding this comment.
Although we will need to make sure we at least warn people who upgrade if they were using that capability.
There was a problem hiding this comment.
Nice. I'll also check how my algorithm behaves in with relative paths in css files. I'm not sure what the behaviour should be, since if the stylesheet gets a host prepended, the images referenced by it can continue to be relative, because iirc they use the host of the stylesheet
In exchance it makes some other tests more exhaustive.
|
Before merging this I'd like ppl to give it a try to see if there someone finds a regression. If anyone wants to help and wants to use things like |
|
I just opened #54 which accidentally fits very well with the direction of this PR, although the two will have to be merged at some point. |
|
@bendemboski I hit a dead end with this TBH. When I fix a test another breaks. This regex approach is pretty hard to maintain, but on the other hand a proper parsing is pretty slow. |
|
Oh, okay. The aspect that fits well with #54 is having different processor functions for different file formats, so side point and not the main thrust of this PR. If it looks like this is a dead-end, then I won't worry about merging, etc., in advocating for getting my PR merged 😄 |
Hi.
First of all, thanks for this addon that has served to well in the ember community.
It has however a few issues still:
Also, performance in very big apps is a bit slow due to the complexity of the regular expression being used.
I decided to take a spike and see of I could find a way of accomplish the same task while ironing the broken edge cases and maintaining performance, and I think I did.
It successfully handles the
srcsetsyntax (#51) and replaces with absolute URLs in css (#39).Good news, is also around 2x faster!
If you are available, I'd like to discuss (perhaps on Slack?) one edge case of my approach that is making a test fail, and see if we can come up with more possible edge cases.
In my approach I'm applying it only on JS files, but I think that it would work for any kind of file.
I've also made the js-perf example 6x bigger to have a more realistic scenario (is not uncommon that a unminified app surpases 5mb (I've seen 12mb!), so the ~400K example wasn't representative enough of the kind of file sizes this addon handles (performance seemed linear in both approaches tho)