Skip to content
This repository has been archived by the owner on Nov 9, 2024. It is now read-only.

Help test out Tippy v5! #530

Closed
atomiks opened this issue Jun 30, 2019 · 37 comments
Closed

Help test out Tippy v5! #530

atomiks opened this issue Jun 30, 2019 · 37 comments

Comments

@atomiks
Copy link
Owner

atomiks commented Jun 30, 2019

Hi everyone,

The next version of Tippy.js is coming in August. In order to ensure it's as stable as possible, I need you to help test it out.

Poll

How are you using the tippy.js package? Answer here if you get time! #550

Goals

  1. Improve developer experience with warnings without bloating production bundle size
  2. Massively reduce core size and make the library more treeshake-able
  3. Allow new feature additions to be added separately without increasing bundle size
  4. Improve naming consistency and usage

Highlights

  • ⬇️ 26% smaller download size (5.8 kB minzipped including core CSS)
  • ⬇️ 46% smaller parse size (16.9 kB minified including core CSS)
  • ⬇️ 61% smaller core CSS (0.6 kB minzipped) + smaller external animation files
  • ⚙️Development and production versions for better developer experience
  • ✨New animation variations
  • ✨New touch: ["hold", delay] prop (for long press behavior)
  • ✨New arrow: string | Element values to use your own shape
  • ✨New createSingleton method, supersedes group()
  • 🌸Improved animations integration and documentation, namely fully dynamic transition dimensions when tippy content updates
  • 🌸Improved naming consistency

Installation

Package Managers:

# npm
npm i tippy.js@next

# Yarn
yarn add tippy.js@next

CDN:

<script src="https://unpkg.com/popper.js@1"></script>
<script src="https://unpkg.com/tippy.js@next"></script>

Changes

View all of the details in the PR in progress.

Migration Guide

View guide

The warnings will help a lot with this.

Note to CDN users, it's recommended to migrate using the development file:

<script src="https://unpkg.com/popper.js@1"></script>
<!-- Specify development file -->
<script src="https://unpkg.com/tippy.js@5/iife/tippy.bundle.js"></script>
<!-- 
When you're finished, you can remove everything after @5 
(or when deploying for production) 
<script src="https://unpkg.com/tippy.js@5"></script>
-->

If you were specifying a custom file path import

import tippy from 'tippy.js'; // <— fine
import tippy from 'tippy.js/esm/index.min.js'; // <— breaking

Check the new folder structure and filenames. umd is now cjs (Node) or iife (browser).

If you want the material filling effect back (it's no longer default)

Node:

import tippy from 'tippy.js';
import 'tippy.js/backdrop.css';
import 'tippy.js/animations/shift-away.css';

tippy(targets, {
  content: 'tooltip',
  animateFill: true,
  animation: 'shift-away'
});

Browser:

<link rel="stylesheet" href="https://unpkg.com/tippy.js@5/backdrop.css" />
<link
  rel="stylesheet"
  href="https://unpkg.com/tippy.js@5/animations/shift-away.css"
/>
<script src="https://unpkg.com/popper.js@1"></script>
<script src="https://unpkg.com/tippy.js@5"></script>
<script>
  tippy(targets, {
    content: 'tooltip',
    animateFill: true,
    animation: 'shift-away'
  });
</script>

If you were using arrowType: 'round'

Include the svg-arrow CSS, and use the arrow prop instead.

Node:

import 'tippy.js/svg-arrow.css';

tippy(targets, {
  arrow: 'round'
});

Browser:

<link rel="stylesheet" href="https://unpkg.com/tippy.js@5/svg-arrow.css" />

If you were using followCursor

Enhance the tippy base function with this prop by importing it from tippy.js/extra-props.

Node:

import tippyBase from 'tippy.js';
import enhance, {followCursor} from 'tippy.js/extra-props';

const tippy = enhance(tippyBase, [followCursor]);

tippy('button', {followCursor: true});

Browser:

<script src="https://unpkg.com/popper.js@1"></script>
<script src="https://unpkg.com/tippy.js@5"></script>
<script src="https://unpkg.com/tippy.js@5/extra-props/iife/tippy-extra-props.min.js"></script>
<script>
  tippy('button', {followCursor: true});
</script>

If you were using tippy.group()

Use createSingleton() and import from tippy.js/addons.

Node:

- import tippy from 'tippy.js';
- tippy.group(tippy('button'), { delay: 1000 });
+ import { createSingleton } from 'tippy.js/addons';
+ createSingleton(tippy('button'), { delay: 1000 });

Browser:

<script src="https://unpkg.com/popper.js@1"></script>
<script src="https://unpkg.com/tippy.js@5"></script>
<script src="https://unpkg.com/tippy.js@5/addons/iife/tippy-addons.min.js"></script>
<script>
  tippy.createSingleton(tippy('button'), {delay: 1000});
</script>

If you were using the target option

Use delegate() and import from tippy.js/addons.

Node:

- import tippy from 'tippy.js';
- tippy('#parent', { target: '.child });
+ import { delegate } from 'tippy.js/addons';
+ delegate('#parent', { target: '.child' });

Browser:

<script src="https://unpkg.com/popper.js@1"></script>
<script src="https://unpkg.com/tippy.js@5"></script>
<script src="https://unpkg.com/tippy.js@5/addons/iife/tippy-addons.min.js"></script>
<script>
  tippy.delegate('#parent', {target: '.child'});
</script>

If you were using the showOnInit option

It's now named showOnCreate, to match the onCreate lifecycle hook

If you were using the size option

It's been removed, as it's more flexible to just use a theme and specify the font-size and padding properties.

If you were using the included themes

  • google is now material

If you were creating custom themes

  • [x-placement] attribute is now [data-placement]
  • .tippy-roundarrow is now .tippy-svg-arrow
  • .tippy-tooltip no longer has padding on it, rather the .tippy-content selector does.
  • .tippy-tooltip no longer has text-align: center

If you were using default animations or creating custom animations

  • shift-away, shift-toward, scale and perspective need to be imported separately now.

Node:

import 'tippy.js/animations/scale.css';

Browser:

<link
  rel="stylesheet"
  href="https://unpkg.com/tippy.js@5/animations/scale.css"
/>
  • Make sure your visible state has no translation (of 0px, instead of 10px like before).

If you were using virtual reference elements

Pass a placeholder element in instead of a plain object:

tippy(document.createElement('div'));

You can overwrite its getBoundingClientRect method, just like a regular object. Make sure this placeholder element does not get appended to the document though.

If you were calling instance.set() / tippy.setDefaults() / accessing tippy.defaults

- instance.set({});
+ instance.setProps({});
- tippy.defaults;
+ tippy.defaultProps;
- tippy.setDefaults({});
+ tippy.setDefaultProps({});

Types

  • Props is not Partial anymore, it's Required
  • Options removed (use Partial<Props>)
  • BasicPlacement renamed to BasePlacement
@RobbieTheWagner
Copy link

RobbieTheWagner commented Jun 30, 2019

@atomiks I tried installing in Shepherd and got Uncaught ReferenceError: process is not defined from where it does if (process.env.NODE_ENV !== "production") {. Is tippy meant to run in node?

I'm using import tippy from 'tippy.js' FYI

@atomiks
Copy link
Owner Author

atomiks commented Jun 30, 2019

Yeah. I don't really know how to handle browser ESM, current solutions for that are @pika/web and its CDN.

@RobbieTheWagner
Copy link

@atomiks it seems like the process stuff is on the rollup side of things. Shouldn't that stuff get executed before the built output?

@atomiks
Copy link
Owner Author

atomiks commented Jun 30, 2019

The process variable is there for bundlers to distinguish development vs production versions. When NODE_ENV=development, you get helpful warnings when things go wrong (i.e. parcel or webpack dev server). When building for production, all of that stuff gets stripped out to conserve bundle size and runtime perf since it's not necessary. That's not possible in the browser and why I don't see the point of <script type="module"> yet.

@RobbieTheWagner
Copy link

@atomiks hmm, well we are using rollup ourselves to build Shepherd, so how would we get this to work as part of our build?

@atomiks
Copy link
Owner Author

atomiks commented Jun 30, 2019

Ah I see. So Rollup breaks when bundling it? I would've assumed since it's Node-based it would just work 😖

You could use rollup-plugin-replace to replace those whole process.env.NODE_ENV !== 'production' expressions with false I guess.

@RobbieTheWagner
Copy link

@atomiks yeah, I use process.env to do different builds in my rollup config as well, but for some reason when doing import tippy from 'tippy.js' the process part is not passed in there I suppose.

Perhaps we could add a check, in the tippy code, and if process is undefined default process.env to 'production'? I'd be happy to open a PR for this, if you are in support of it.

@RobbieTheWagner
Copy link

FWIW using rollup-plugin-replace also works, which I tried here shipshapecode/shepherd#420 but I think having a sane default to production would help here. Let me know your thoughts!

@atomiks
Copy link
Owner Author

atomiks commented Jun 30, 2019

Perhaps we could add a check, in the tippy code, and if process is undefined default process.env to 'production'? I'd be happy to open a PR for this, if you are in support of it.

think having a sane default to production would help here. Let me know your thoughts!

Not sure what you mean here, how would it work?

@RobbieTheWagner
Copy link

@atomiks somewhere before this:

if (process.env.NODE_ENV !== "production") {

We could do something like:

if(!process) {
  process = {env: {NODE_ENV: 'production'}}
}

Not sure exactly, but something like that. I'm not even really sure where these checks are in the code, but if we guard against process being undefined, we should be okay here, I would think.

@atomiks
Copy link
Owner Author

atomiks commented Jun 30, 2019

Hmm would like to avoid environment polyfills like that as it adds a side effect that could cause problems for the consumer somehow, I think the replace plugin thing is fine for bundling it as a dependency, better keep the error explicit

@RobbieTheWagner
Copy link

@atomiks I suppose I am just confused then. Under what circumstances is process.env.NODE_ENV defined? Won't everyone who uses the import tippy from 'tippy.js' syntax hit this error?

@RobbieTheWagner
Copy link

It looks like using this allows using globals like process in rollup https://www.npmjs.com/package/rollup-plugin-node-globals

I was also reading if you explicitly define process it might help.
const process = require('process');

@atomiks
Copy link
Owner Author

atomiks commented Jul 1, 2019

@atomiks I suppose I am just confused then. Under what circumstances is process.env.NODE_ENV defined? Won't everyone who uses the import tippy from 'tippy.js' syntax hit this error?

Tools like create-react-app do this automatically. When using a custom webpack config, you can use webpack DefinePlugin or set the env, and Parcel sets NODE_ENV=development|production for you.

Then your minifier (e.g. Terser) will use dead code elimination to remove those whole blocks in production so they can be used in the browser without the error.

Haven't tried Rollup itself, but this is more for apps instead of libraries.

Libraries already do it the way I'm doing it. It's expected you'll set up your bundler to make this work: https://unpkg.com/react-window@1.8.4/dist/index.esm.js

Edit: It seems you're referring to the error occurring inside the browser itself, rather than when Rollup bundles the files for publishing on npm? https://github.com/shipshapecode/shepherd/blob/master/rollup.config.js#L71

In Parcel I just put a <script> at the top of the HTML file:

process = { env: {} }

And that's only because of the __DEV__ expression babel plugin I'm using, it seems Parcel doesn't like it. Parcel will replace process.env.NODE_ENV normally if that's how it is statically. So it should just work if you're installing tippy from npm.

@RobbieTheWagner
Copy link

@atomiks

Tools like create-react-app do this automatically. When using a custom webpack config, you can use webpack DefinePlugin or set the env, and Parcel sets NODE_ENV=development|production for you.

So if you are not setting NODE_ENV in your build or using one of the tools you mentioned, you will hit this error.

Haven't tried Rollup itself, but this is more for apps instead of libraries.

While rollup is typically for libraries and webpack more for apps, won't you hit this error in webpack as well, if you didn't define NODE_ENV?

Edit: It seems you're referring to the error occurring inside the browser itself, rather than when Rollup bundles the files for publishing on npm?

The rollup build succeeds, but it does not strip out the process.env stuff because it's undefined. Since it is not stripped out, it makes it to the npm output, which then throws in the browser.

So it should just work if you're installing tippy from npm.

It is not though. import tippy from 'tippy.js' is causing this error, so the output from tippy installed from npm still includes these checks. I would think import tippy from 'tippy.js' would import built files, with these things already stripped.

@atomiks
Copy link
Owner Author

atomiks commented Jul 1, 2019

So if you are not setting NODE_ENV in your build or using one of the tools you mentioned, you will hit this error.

Not necessarily - just if your bundler isn't replacing process.env.NODE_ENV with a value. As I mentioned, Parcel does it automatically, and webpack via DefinePlugin (I think). I think every framework's CLI already does it for you. It works fine with create-react-app.

While rollup is typically for libraries and webpack more for apps, won't you hit this error in webpack as well, if you didn't define NODE_ENV?

You'd need DefinePlugin I believe (haven't tested w/ webpack except for CRA). I think it's a safe assumption that the user needs to set this up as it's an important part of the dev experience these days.

The rollup build succeeds, but it does not strip out the process.env stuff because it's undefined. Since it is not stripped out, it makes it to the npm output, which then throws in the browser.

Yeap that's where replace-plugin comes in for IIFE/browser builds. For ESM/CJS you can assume a bundler environment with this setup.

It is not though. import tippy from 'tippy.js' is causing this error, so the output from tippy installed from npm still includes these checks. I would think import tippy from 'tippy.js' would import built files, with these things already stripped.

Parcel doesn't error out because it replaces the expression for you.

It boils down to:

  • If using ESM/CJS versions, you need a bundler that can replace the process stuff in the dev server (browser) and set NODE_ENV for production/dev differentiation. All major framework CLIs already do that.

  • If using IIFE, the process stuff is stripped out entirely for the browser because it's done based on a per-file basis, rather than an environment variable basis. Because you can only use a single import path import tippy from 'tippy.js' for ESM but you can choose which file path to use for browser <script> tags.


Check the react-window link. All the process stuff is still there because the library assumes your bundling methods handle it, which we're assuming here. If the user doesn't use a bundler they can just use the IIFE versions and not care about it

@atomiks atomiks mentioned this issue Jul 1, 2019
@atomiks
Copy link
Owner Author

atomiks commented Jul 1, 2019

Oh and the reason we can't do this is because it doesn't work with ESM, only CJS format. React doesn't provide ESM at all atm probably for that reason


Let's say the user somehow wants to use ESM and can't use any tools to fix, or even polyfill, the process stuff. Then we can provide a minified file to import from which is equivalent to the production version:

import tippy from 'tippy.js/esm/tippy.bundle.min.js'

I don't even know how they're using that without a proper bundler setup tho. And the reason I removed the minified versions for ESM/CJS is cuz they add tons of files and therefore node_modules bloat which is contributing to the node_modules = black hole meme

@RobbieTheWagner
Copy link

@atomiks

I suppose this just isn't really making sense to me.

If using ESM/CJS versions, you need a bundler that can replace the process stuff in the dev server (browser) and set NODE_ENV for production/dev differentiation. All major framework CLIs already do that.

Seems very opinionated and I'm not sure why we need to assume this. There are tons of potential situations where you'll use a modern JS import, like you do in all JS development these days, rather than manual script tags.

IMHO we should be defaulting to assuming we are running in the browser. How often do we need tooltips in a node environment?

At the very least we should ship all the formats bundled for both node and browser, rather than defaulting to node. I consider this issue a major breaking change, as anyone using the import and not using React or Parcel or one of the environments that sets all this for you, will 100% definitely hit this error.

@RobbieTheWagner
Copy link

I think I am still confused why built output contains process.env at all. I think my suggestion here is the built files should strip all that out, without relying on assumptions of bundlers setting process.

@RobbieTheWagner
Copy link

All the checks in the build output could be fixed by simply doing if (process && process.env.NODE_ENV !== "production") {

@atomiks
Copy link
Owner Author

atomiks commented Jul 1, 2019

I think there's a lot of confusion here 😓

I think I am still confused why built output contains process.env at all. I think my suggestion here is the built files should strip all that out, without relying on assumptions of bundlers setting process.

The entire point is for development warnings to help you when developing.

node_modules/tippy.js/esm/tippy.bundle.js:

export default function tippy(targets) {
  if (process.env.NODE_ENV !== 'production') {
    // contrived, just pretend it's real
    if (typeof targets !== 'string') {
      console.warn('Woops you did something wrong. Here\'s a fix...');
    }
  }

  return targets;
}

your-app.js:

import tippy from 'tippy.js'

tippy(1000);

// You get a warning in DEV mode.

Now when building for prod and setting NODE_ENV=production, webpack/Parcel does something like this:

export default function tippy(targets) {
  if ('production' !== 'production') {
    // contrived, just pretend it's real
    if (typeof targets !== 'string') {
      console.warn('Woops you did something wrong. Here\'s a fix...');
    }
  }

  return targets;
}

Then your minifier (e.g. Terser) which runs as the next step in your plugins sees that the whole block can be deleted because it's dead code (the block evaluates to false and will never run. So your production build ends up like this:

function t(x){return x}

All that bloat is gone. Leaving the process in there is the whole point, to be able to achieve this differentiation between DEV (for the developer) and PROD (for the end users).

All the checks in the build output could be fixed by simply doing if (process && process.env.NODE_ENV !== "production") {

That would cause a ReferenceError (and break the dead code elimination because it can't be statically analyzed properly anymore)

@RobbieTheWagner
Copy link

@atomiks sure, I understand. We need to heavily document how to set process.env.NODE_ENV in all possible build tools then or people will hit this error.

I've tried using rollup-plugin-replace, as suggested here rollup/rollup#487 (comment)

This works, but doesn't work correctly. No matter what I pass to this, it removes all the process.env checks, so somehow it is always being set to production, regardless of what I pass.

@RobbieTheWagner
Copy link

@atomiks okay, I fixed the replace 🎉 shipshapecode/shepherd#420

We should document how to do this in webpack, rollup, and other build tools that do not do it automatically like parcel does.

@atomiks
Copy link
Owner Author

atomiks commented Jul 1, 2019

Yep I agree the documentation will be important.

I think there are 3 main bundlers:

  • Rollup (use replace plugin)
  • Webpack (use DefinePlugin plugin)
  • Parcel (auto)

We can provide info for these on how to make it work.

I think Browserify is dead and Browser ESM doesn't work properly with external deps like popper.js so I feel like that's a dead-end anyway.

@RobbieTheWagner
Copy link

@atomiks sounds good to me! Thanks for bearing with me through figuring this out. I was pretty ignorant to the whole process 😬

@RobbieTheWagner
Copy link

RobbieTheWagner commented Jul 2, 2019

@atomiks was there something setting text-align: center for .tippy-tooltip before that is not now?

@atomiks
Copy link
Owner Author

atomiks commented Jul 2, 2019

@rwwagner90 yes it was removed since it makes HTML content centered. Trying to keep the CSS minimal.

@RobbieTheWagner
Copy link

Perhaps we should call out the style changes, so people can make sure anything they relied upon is updated?

@atomiks
Copy link
Owner Author

atomiks commented Jul 2, 2019

I added it to the Migration Guide

@RobbieTheWagner
Copy link

@atomiks is everything in the v5 alpha pretty stable? If no APIs should change any further, I would like to go ahead and use the alpha for a new Shepherd release.

@atomiks
Copy link
Owner Author

atomiks commented Jul 6, 2019

Some more stuff removed and bugs fixed in v5 branch that haven't been released. Maybe wait till beta

@RobbieTheWagner
Copy link

@atomiks okay sure thing. Still looking at July 20th?

@atomiks
Copy link
Owner Author

atomiks commented Jul 6, 2019

For final release? Yeah. Beta in the next day or so maybe?

Doesn't seem like too many people are going to test it out anyway so w/e lol. I'll just do what I think is best as possible.

@RobbieTheWagner
Copy link

Sounds good! Looking forward to the beta 😃

@RobbieTheWagner
Copy link

@atomiks would you be interested in me creating an Ember wrapper for tippy?

@atomiks
Copy link
Owner Author

atomiks commented Aug 4, 2019

@rwwagner90 sure! Would be best if we could also publish it under @tippy.js/ember. There is an open issue about it various framework wrappers now.

Anyway I'm gonna close this issue because I reminded myself of this tweet https://twitter.com/acemarke/status/1116524847771705345

@atomiks atomiks closed this as completed Aug 4, 2019
@RobbieTheWagner
Copy link

@atomiks I'll work on creating something for Ember sometime soon. I'll let you know.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants