mrsum / webpack-svgstore-plugin

Simple svg-sprite creating with webpack
https://www.npmjs.com/package/webpack-svgstore-plugin
200 stars 92 forks source link

Webpack 4/5 compatability #168

Closed nadavsinai closed 1 year ago

nadavsinai commented 5 years ago

Hello again .... now we need webpack 4 compatibility

nadavsinai commented 5 years ago

I am changing compatibility to node 7.6+ (use of async/await)

lgordey commented 5 years ago

@nadavsinai Actually I didn't look at the code, but we need to make the tests green ¯\_(ツ)_/¯

nadavsinai commented 5 years ago

@lgordey tests fail because they ran on node 4/6. 4 is at EOL. 6 is pretty much dead too. I don't know how to make travis run the tests on node 8/10 which is what matters today.

nadavsinai commented 5 years ago

ah. found .travis.yml changed that now...

nadavsinai commented 5 years ago

Ok can't do everything from the github web UI... will take the code locally and fix that soon but gotta go now...

nadavsinai commented 5 years ago

hey @lgordey , finally got to it. I did some more work, not only fixing the tests. The plugin took way too much time from out builds I started researching. I found that I actually did a bad merge the other day leaving both the async work we used to do and the new sync work I got by mergeing from develop (after PR #165 ) I now fixed that. another major thing I found is that the emit hook is called more than once!! and the work we're doing should/could be done in compilation.additionalAssets hook, so I changed that too. another issue is with the client side svgXhr loader - I believe we should not do anything about the baseUrl - the filename we request is relative not absolute, there is no / in the beginning and all browsers will request that from the current page as source - I actually find that adding the baseUrl and making that request for an absolute URL will not work in some cases .Definitely this is a major - with breaking changes to explain. I don't mind to update the readme too unless you @mrsum want that for yourself.

nadavsinai commented 5 years ago

TLDR: I added the baseUrl addition again - this time as an option, that default to false.

original comment: another thing about the svgXhr, if a user wants to use absolute url, there is nothing stopping anyone from submitting any url now, this can actually be useful for apps where CDN is involved (static files apart from "server bound" html) so the "backwards compatible" way would be was:

svgXhr(__svg__);

now (the base url "parsing" is done out of scope of svgXhr):

let baseUrl;
if (typeof baseUrl === 'undefined') {
    if (typeof window.baseUrl !== 'undefined') {
      baseUrl = window.baseUrl;
    } else {
      baseUrl =
        window.location.protocol +
        '//' +
        window.location.hostname +
        (window.location.port ? ':' + window.location.port : '');
    }
}

const withBaseUrl =  {fileName:baseUrl + '/' + __svg__.fileName};
svgXhr(withBaseUrl);

another possbility if you find usescases for that addition for baseUrl (which I really don't understand the need for) - then we can add a property to that options object that allows to escape that baseUrl addition.

nadavsinai commented 5 years ago

@mrsum @lgordey tests passed back in Oct 18, i'm just about to do another update (to update globby ) - isn't time to merge this? if it's difficult for you to support this OSS I suggest to take over.

nadavsinai commented 2 years ago

I don't know if there's anyone following.. we still use this tool internally here at Philips so updated again to work with Webpack 5...

mrsum commented 2 years ago

Guys, good news. Fresh version is coming. Compatible with Webpack 5. Stay tune

nadavsinai commented 1 year ago

The fork diverged, we will review the main branch activity, for now closing the PR