filamentgroup / loadCSS

Load CSS asynchronously
MIT License
6.72k stars 534 forks source link

Required node version #319

Closed eps1lon closed 2 years ago

eps1lon commented 4 years ago

In #305 you started requiring node version 11.9 without any code changes. Node 11 has already reached its end of life while 10.x is still LTS.

Is there any reason besides the used version in CI that prevent usage in 10.x?

Otherwise this library does not work with the default netlify configuration.

scottjehl commented 4 years ago

Hi! Thanks for the feedback. I am totally open to setting the node version to current if that works for dependencies here. As it is, the current recommended workflow is a snippet in the readme file anyway! :)

Any reason I shouldn't set it to latest (14 looks like)? Thanks

eps1lon commented 4 years ago

Any reason I shouldn't set it to latest (14 looks like)? Thanks

~As I understood node release cycle 14 is only meant for libraries to test integration not for apps in production. So if you set it to 14 most apps won't be able to install it.~ 13 is meant for library integration. 14 is meant for production.

12 would be the active lts. 10 is still maintained though and since this code is meant to work in the browser you probably don't rely on any features in node 11 or 12.

We used it for lazy loading CSS of 3rd party libraries for certain components. It was quite handy since the user of the component didn't need to make sure they setup dependencies which can lead to dead CSS if the component is removed but not the dependencies e.g.

export default function AppSearch() {
  React.useEffect(() => {
    const styleNode = loadCSS(
      'https://cdn.jsdelivr.net/docsearch.js/2/docsearch.min.css',
      document.querySelector('#app-search'),
    );

    return () => {
      styleNode.parentElement.removeChild(styleNode);
    };
  }, []);
}

As it is, the current recommended workflow is a snippet in the readme file anyway! :)

Sure but this change is a good example why having this as a dependency is preferred. I never looked into the best way of lazy loading css and trusted that you did. By inlining it, it's now more code that can become outdated.

scottjehl commented 4 years ago

Sounds great. We intend to keep the function for programmatic loading, of course, so that will continue to work as expected.

Thanks for the tip on the preferred version.

maxp-hover commented 4 years ago

Hey, just to give my personal anecdote of how I came across this thread -

Deploying a new Rails 6 app to Heroku, the default Node version is only 10.15. However this library (which I use) requires 11, so now I need to do a complicated dance to get my app properly deployed.

I would request that we please change it back to Node 10 dependency, it is better to make it continue work on older Node versions

eps1lon commented 2 years ago

Node 10 has reached its end-of-life so this issue is no longer relevant.