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

upgrade dependencies #113

Closed
wants to merge 1 commit into from
Closed

upgrade dependencies #113

wants to merge 1 commit into from

Conversation

Fil
Copy link
Member

@Fil Fil commented Sep 11, 2020

@Fil Fil requested a review from mbostock September 11, 2020 15:14
import Context from "./context.js";
const tape = require("tape-await");
const {Delaunay} = require("../");
const Context = require("./context.js");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This effectively changes the tests to run against the built version rather than the source, right? Asking just in case it wasn't intentional. One disadvantage of this is when you develop something continuously, you have to rerun the build constantly instead of having instant feedback.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes. It was intentional to normalize to what happens in the other D3 modules, but I didn't realize the implication. If it's a drag, we should find a better solution.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A variant here #115

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the old way, the build was happening implicitly on the fly because this module uses esm. The other D3 modules don’t currently use esm; they only use Rollup. And the thinking was it’s better to test the built bundle because that’s what consumers of the library will be using (and historically there have been some bugs introduced by bundlers, but that’s less of a problem these days).

I think it’d be great to switch to native support for ES modules but we’re not quite there yet. I think I’m ambivalent about whether it’s worth the trouble to switch all the D3 modules to use esm for testing or to run Rollup before tests. Rollup should be fast enough for a small library that the difference is negligible.

@Fil
Copy link
Member Author

Fil commented Apr 26, 2021

superseded by ce37643

@Fil Fil closed this Apr 26, 2021
@Fil Fil deleted the deps branch April 26, 2021 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants