search

atomiks / tippyjs

Tooltip, popover, dropdown, and menu library
https://atomiks.github.io/tippyjs/
MIT License
11.91k stars 518 forks source link

ESM with Importmaps #990

Closed coder2000 closed 2 years ago

coder2000 commented 2 years ago

Bug description

I know importmaps aren't on most peoples horizon but it is coming up quick with Rails developers and the release of Rails 7. Currently tippy requires a build environment like webpack, rollup or esbuild however with importmaps you just include the final ESM build from a CDN so there is no build step.

When I try to use tippy in this fashion it fails with Uncaught ReferenceError: process is not defined.

Reproduction

As I can't include the ESM shims this pen will only work on Chromium based browsers. The error will be visible in the browser console not the codepen for some reason.

https://codepen.io/coder2000/pen/mdMRgLL

atomiks commented 2 years ago

Can you polyfill window.process = { env: { NODE_ENV: 'development' }} etc? This way you will continue to get useful warnings/error messages while developing.

coder2000 commented 2 years ago

Just tried it in my app and in the code pen. No change, unless I am doing it wrong.

atomiks commented 2 years ago

The polyfill will need to be added before all of your scripts; placing it in <head> in an inline script tag would work.

coder2000 commented 2 years ago

Ok, that works, https://coder2000.github.io/. Codepen it seems doesn't like to do importmaps.

sedubois commented 2 years ago

I know importmaps aren't on most peoples horizon but it is coming up quick with Rails developers and the release of Rails 7.

I'm one of those Rails 7 developers and hit this error process is not defined. Rather than polyfill, in my view a better fix would be to have a production-ready bundle where the dev code is stripped out for compactness. Would that make sense?

gregblass commented 2 years ago

+1 for proper support/documentation here.

Rails 7 introduced importmaps by default, and I must say - it is pretty awesome to not have to deal with a build environment.

You may want to consider adding some information to the documentation on how one might use Tippy using importmaps, as more people may start adopting it as time goes on.

dougc84 commented 2 years ago

Seconded @gregblass - I just spent an hour trying to make tippy work on a new Rails 7 app only to end up here.

ericraio commented 2 years ago

same here for tippyjs

mildred commented 2 years ago

The polyfill, while it can make it work, is not really adequate. I'm coming here because tippy is a transitive dependency and for a library to require a global setting like this is not a good practice.

Could the code be updated to check for typeof(process) !== 'undefined' like explained in https://jspm.org/docs/cdn#universal-module-semantics ?

When accessing environment-specific globals like process in Node.js, always use a guard like typeof process !== 'undefined'as they won't necessarily be available in other environments. Ideally, rather import these modules where possible - import process from 'process'.

atomiks commented 2 years ago

Sorry I know this is annoying :\

Any type of guard like that always seemed to break the dead code elimination by bundlers when I checked (defeating the purpose of the check), which causes the warning messages to end up in production bundles inflating the size.

woodardj commented 1 year ago

Defining window.process in the browser to avoid a problem with node environment bundlers feels like a really non-solution, and could easily lead to issues with other libraries. @atomiks when you say it "inflates the size" of a production bundle, what are we talking? A couple of kb? I'd much rather have this work out of the box with a proper environment check as @mildred references than potentially save microseconds of load time.

santiagodoldan commented 1 year ago

It would be great to use this library via importmap out of the box, I agree that defining the process object seems too hacky IMHO

BakiVernes commented 1 year ago

Any updates on this? @atomiks would a PR on this be considered?

edolix commented 6 months ago

@atomiks can we help in any way? Following the last comment: would a PR on this be considered?

Thanks!