kensho-technologies / orama Goto Github PK

Currently, our unit tests live adjacent to the files they test. This makes sense for testing pieces in isolation, but we never verify whether we are exposing a given piece as part of the public API. During cleanup, it's important that we don't unintentionally remove parts of the public API outside of a major release, so I'd like to establish these guarantees.

We can do this by having tests import from the top-level public API where possible. These tests could be moved to a top-level test/ directory, with the Mocha config updated accordingly, which is more consistent with how we tests other projects internally. (Any files which test orama internals -- not exposed at the top-level -- could stay where they are, for now.)

This code is surprising:

It's possible for up to 3 canvas re-renders to be triggered on a single update. Maybe there's a complicated reason why this is necessary (fit-checking text?), or maybe it was unintentional.

The easiest way to figure this out is probably to profile the re-renders in one of our existing internal uses of orama, and/or see what happens to our internal usage if we remove the cWRP lines.

orama implements a simple window mock for node environments. It's only used in one place internally which should be easy to substitute. However, it's exported at the top-level because of a re-export in utils/, so its removal will be a breaking change.

Currently, all files are named index.js and live in an appropriately-named folder. This was a stylistic choice when orama was first written, but I'd like to align it with the style used internally and more commonly used elsewhere.

d8eb4ad#diff-3e04ec70a0e53882379d13e9afe147c7R53 exposed a previously-hidden bug.

By default, charts should have a transparent background. A white background was incorrectly specified in the default theme, but the bug was masked because the attribute was being selected from the unmerged, user-supplied theme. The default theme needs to be updated accordingly.

To replace oramajs.com.

This can also serve as a sandbox for local development and testing.

• Custom proportion
• Custom height (overriding proportion)
• Custom width (short-circuiting dynamic sizing)

Text that runs off the edges of the canvas is not wrapped or shifted into view.

We might also want to drop support for React <16.3, so that we can use the new lifecycle methods.

• no-nested-ternary
• no-param-reassign
• no-shadow
• no-underscore-dangle
• react/forbid-prop-types
• react/no-access-state-in-setstate
• react/no-deprecated
• react/no-unused-prop-types
• react/no-unused-state
• react/sort-comp

This will allow us to test #88.

When writing #143, I forgot that orama allows consumers to pass a custom tooltip component, so the changes I made to the API of the default tooltip component should not have been backwards-incompatible. This caused an unintentionally breaking change in 2.0.1.

We can keep most of that refactor, but we need to revert the component API, which will require the measuring code to be pulled back out into another component.

To reproduce, in the Brush sandbox example:

1. Click and hold the brush
2. Move the mouse outside of the brush bounds
3. Move it back inside the bounds

Here's the stack trace:

Uncaught TypeError: Cannot read property 'x' of null
    at mouseDrag (Brush.js:108)
    at handleChart (Brush.js:169)
    at Object.onUpdate (Brush.js:198)
    at handleCanvasInput (index.js:34)
    at Object.onUpdate (index.js:68)
    at CanvasInput.<anonymous> (index.js:126)

Might have been introduced in 9c50642. @nabeel-, would you mind investigating?

To make sure we don't regress in performance for very large data sets. (The current sandbox examples all use small ones.)

Is it possible to show Y units and label on the right side instead of left?

It would be great to copy over all the examples from the old docs site, as well as any new ones we'd like to add.

Currently, we scale all canvases by 2x, which makes them look sharp on displays with a pixel density of exactly 2. But we don't need to incur this performance cost on low-density displays, and we should scale more on displays with an even greater density.

Here's a good example of how to do this, and here's some info on pixel density and backing stores.

I've encountered a problem but I'm still trying to reproduce it outside my app.
My chart is inside several containers with fixed > absolute positions.
It's tooltip is being offset by the chart's relative parent 'left' position.
I tried studying the TooltipWrapper component but couldn't figure the problem that easily.
I'll continue trying to reproduce the problem in codesandbox.io and place the link here when I'm done.

Generally it's not good practice to call the Function constructor. Like eval, it's a fragile operation with many associated security vulnerabilities, and there is usually a better solution.

We should figure out why we're calling the constructor here, and come up with a better solution.

I think I broke areas at some point during the refactor. Looking at the sandbox, the first point in each area is missing.

We probably need to cherry-pick the sandbox onto an old version, rebase everything else on top of that, then bisect.

We don't use this internally anymore so we should make it clear that we don't intend on maintaining or supporting it.

As described in #21, we could have Babel output a second build (probably to es/), configured not to transpile ESM to CJS as the current build does. Setting {"module": "es"} in package.json would allow ESM-aware module systems (e.g. webpack) to load that build instead of the CJS one.

The main benefits here are tree-shaking and less module interop glue code since webpack is able to concatinate ESM.

Ideally we fix #24 first.

For example, from the sandbox:

This only happens for some of the charts.

The image renderer is fairly large and isn't used within orama -- it's only exported as a utility. It seems out of scope for the library, and we don't use it internally, so I think we should remove it in v2.

I wish I would be able to take an action when the user hovers or clicks a data point.
However I did not found anything in the docs on how to do this. Is this available?
Is there any design issues on enabling this?

My use case is this:
I have an area chart that I wish I would display additional information when the user hovers a data point. I understand there is the tooltip component but the information would be better displayed in a larger and fixed container. (The container will display the details of the last hovered/clicked even when the user leaves the chart.)

And congratulations again on this library Luis. I was using chart.js since I had no available time to study d3.js properly. But I quickly found myself trying to compose different chart types and with this library this is a complete breeze. Thanks!

Profiling the stress test in #123, I noticed that 40% of each frame is spent calling isPointInStroke to compute which datum the mouse is hovering over. (This use of isPointInStroke is also the only reason that orama requires a polyfill to support IE 11, AFAIK.)

Instead of this canvas-based approach, we could collapse all data into a voronoi diagram, and use it to determine the closest datum to a given mouse position, which would be relatively instantaneous. Here's an example of that approach.

(This would change the existing hover behavior, arguably for the better.)

This file is unused, other than a re-export at the top level. I don't think it's worth keeping in the library.

We could steal some from the old docs site.

This would allow us to test for tooltip regressions.

Now that v2 has dropped support for React < 16.3, we can use getDerivedStateFromProps and createRef. 🎈

Wrapping the sandbox in React.StrictMode will help us catch issues.

Probably converging orama's style with Kensho is too much work to be worthwhile, but we could see how far we'd get by adding our standard ESLint config, running an autofix, and disabling outstanding failing rules.

This would at least run the codebase through Prettier, and might catch some bugs.

The instance variables representing mouse data should be replaced with state to ensure compatibility with React's upcoming async mode.

Most files should either contain a single default export or several named exports. Files in the former category should be named according to their export, and those in the latter category should be given a plural name that describes the collection of their exports (e.g. "utils").

The only case where a file should have both a default and named exports is when the default export is the sole export consumed elsewhere, and the named exports are exported for testing purposes.

Currently, orama uses a mix of conventions, so it would be nice to clean this up.

If you click and hold a brush range, leave the brush bounds, and re-enter the bounds somewhere else, the brush does not snap to the new location, but instead offsets from the old one.

Compare 2.0.0-alpha.1:

To 1.5.1:

We should ensure the old behavior is preserved.

There are a few CJS requires/exports sprinkled around the source. We should replace these with ESM imports/exports. Otherwise, outputting an ESM build (as described in #21) won't be as effective since the remaining CJS will de-opt tree-shaking.

For the sake of reducing bundle size.

Unlike our internal code, we shouldn't assume a shimmed ES2015 environment in orama, so let's stick to ES5 methods for now.

We should use a native Portal rather than the unstable API we used when we supported React 15: