Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add compiled dist version to npm package? #39

Closed
TremayneChrist opened this issue Feb 22, 2019 · 13 comments · Fixed by #63
Closed

Add compiled dist version to npm package? #39

TremayneChrist opened this issue Feb 22, 2019 · 13 comments · Fixed by #63
Labels
enhancement New feature or request question Further information is requested

Comments

@TremayneChrist
Copy link
Member

Wondering whether to add a compiled ES5 version of the library which polyfills the global ResizeObserver on the window.

This makes it easier to integrate into older projects.

Would this be helpful?

@TremayneChrist TremayneChrist added the question Further information is requested label Feb 22, 2019
@marval2
Copy link

marval2 commented Mar 8, 2019

Hey, nice work on this! I stumbled upon ResizeObserver and your polyfill just recently. Planning on using it to detect element height changes.

I'd say yes, add to npm packages.

@TremayneChrist
Copy link
Member Author

Hey @marval2, thanks, that's great to hear!

It's good to have your input on this. Currently I have concerns over polyfilling the global scope as it becomes too easy to just throw it in and forget about it. This means the native RO could be overridden, even when it's available to use. Saying that, I'm still leaning towards adding it in. I also think it's required and there are still options to only load the polyfill, if RO isn't available.

@TremayneChrist
Copy link
Member Author

Some options for output directory name...

dir: dist, global, polyfill, compiled, bundled, es5.

<script src="@juggle/resize-observer/<dir>/resize-observer.js"></script>

@marval2
Copy link

marval2 commented Mar 9, 2019

I'd say add it in, it won't hurt and as you said there are other options to load. My vote goes for dist

@JabX
Copy link

JabX commented Apr 17, 2019

Just found out about the whole ResizeObserver thing and your polyfill, I tried it and it's great, definitely gonna use it in the near future.

I was a bit surprised to see that IE11 is supported while there isn't any ES5 version of the library shipped with the package (got a syntax error while loading my app on IE, that was weird). Anyway, unless there is some feature that depends on ES2015 (and since it works on IE, I doubt it), you should simply switch your Typescript target to ES5 so that your consumers (like me) won't need to configure specifically their bundler or whatever to use the library.

A bundled version of the library (in ES5 too) could be useful for a quick import in a project that doesn't use a bundler. I don't think it's reasonable that any version of the library polyfills the global ResizeObserver by default, especially since your goal is to be as close as possible to the spec, instead of say the Chrome implementation that might lag behind it. The spec doesn't look mature enough for this.

@TremayneChrist
Copy link
Member Author

Hey @JabX, that's great!

I agree that the configuration to get it working in IE isn't the easiest and should be documented better (#47). The reason for not releasing in ES5 is because the library becomes bloated and slower in browsers which support ES2015. It's also nicer to import ES modules in modern code, so I'd like to keep the library this way.

The "dist" version would be a quick-win drop and go file that would polyfill the global ResizeObserver - The library is fully backwards compatible with Chrome's version, so if it's used this way, it shouldn't be a problem. Also, feature detection could be used to only load the polyfill when necessary.

The latest spec is definitely not mature enough, which is why I'm holding off on #41. The reason for adding in different box sizes is because it enabled live trialing of the feature and didn't affect the original API.

@JabX
Copy link

JabX commented Apr 18, 2019

You could still target ES5 but with ESNext modules, this is what I usually do when building libraries, as it keeps all the module benefits while not requiring any particular packaging config on the customer side.

@TremayneChrist
Copy link
Member Author

TremayneChrist commented Jun 25, 2019

I think supplying a UMD module alongside an ESM module, could help to fix this issue. I don't think releasing a globally polluting bundle is the right way to go.

UMD:

  • Module would be transpiled to ES5 for easy import
  • Works in browsers and bundlers

ESM:

  • Version is future proof
  • Works in recent versions of bundlers
  • Allows for modern CDNs like Pika to be used

Pika is currently serving ESM and UMD bundles 🎉

https://cdn.pika.dev/@juggle/resize-observer/v2
https://umd.cdn.pika.dev/@juggle/resize-observer/v2

@alejandroiglesias
Copy link

I just tried to use the library in our Meteor project and it fails because Meteor only transpiles app code, not libraries. It's the only library I've had this issue with. I suggest releasing a dist/ directory on npm with the ES5 version and point the main key of the package.json file to this path (you can keep the module key as is). You can use UMD for this so that it can still be require'd without polluting the global window.

@TremayneChrist
Copy link
Member Author

Thanks @alejandroiglesias and sorry it's not currently working for you.

I just realised my previous comment was a bit confusing, so I've reworded it a bit. The plan is pretty much exactly as you said, to keep the current module field pointing towards the ESM entrypoint and then change the main field to point to the UMD entrypoint.

There is NO plan to add a drop-in dist package, which always overrides the global scope.

@TremayneChrist
Copy link
Member Author

To be clear the package.json will be updated to:

{
  // Transpiled ES5 module (UMD)
  "main": "./umd/ResizeObserver.js",
  // Original module (ESM)
  "module": "./lib/ResizeObserver.js",
   ...
}

@TremayneChrist TremayneChrist added the enhancement New feature or request label Jul 2, 2019
@TremayneChrist
Copy link
Member Author

@marval2 @JabX @alejandroiglesias

Ok, so actually playing around with this in different bundlers and real-life situations -> umd is basically a pain in some instances and isn't really needed anyway. Must have being 😴 when looking at this before.

@JabX after some ☕️ I'm thinking of using your idea 👍

Would love some feedback on the PR for this #63

Thanks!

@TremayneChrist
Copy link
Member Author

v2.2.0 is now published in ES5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
4 participants