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

Require error #77

Closed
wants to merge 4 commits into from
Closed

Require error #77

wants to merge 4 commits into from

Conversation

tmcw
Copy link
Contributor

@tmcw tmcw commented Dec 5, 2018

Rely on d3-require's special error branch, and switch module entry to source.

@mbostock
Copy link
Member

mbostock commented Dec 5, 2018

I just shipped d3-require 1.2.0 with RequireError.

@@ -3,7 +3,7 @@
"version": "1.3.0",
"license": "ISC",
"main": "dist/notebook-stdlib.umd.js",
"module": "dist/notebook-stdlib.js",
"module": "src/index.js",
Copy link
Member

Choose a reason for hiding this comment

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

I’m guessing this change isn’t intended?

(It’s here so that unpkg loads the minified ES bundle. It’d be nice if we could have a separate endpoint when consuming the source… but that doesn’t exist yet as far as I’m aware.)

Copy link
Contributor Author

@tmcw tmcw Dec 9, 2018

Choose a reason for hiding this comment

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

This was a combination of a yarn/npm bug that I kept running into while working on this branch, and also trying to determine whether we could avoid re-protecting RequireError from terser mangling in every module in the dependency chain from d3-require to the notebook. I think what would capture my intent and yours as well is to specify module: src/index and unpkg: dist/notebook-stdlib. I feel a little icky about the potential of our code depending on pre-bundled and pre-minified dependencies when it could depend on the source.

Copy link
Member

Choose a reason for hiding this comment

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

The problem is that we want two unpkg-specific entry points but unpkg currently only provides one entry point. We want a minified ES module and a minified UMD:

http://unpkg.com/@observablehq/notebook-stdlib?module
http://unpkg.com/@observablehq/notebook-stdlib

This way, both of these are minified:

import {Library} from "http://unpkg.com/@observablehq/notebook-stdlib?module"
<script src="http://unpkg.com/@observablehq/notebook-stdlib"></script>

There’s an open issue unpkg/unpkg#93 for this, but it doesn’t appear to be moving forward quickly, and even if it were, it’s an unpkg-specific solution which is less than ideal.

It would be possible for us to generate a separate minified ES module that doesn’t use the module entry point, but we’d have to rewrite the bare module specifiers to use relative paths and baked-in semver ranges (exactly what unpkg currently does automatically with ?module).

I think most consumers would be fine with a minified module entry. It’s just an extra pain for us because we have to remember to yarn prepare when editing notebook-stdlib even when it’s yarn link’d into notebook-runtime.

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, we could consider not recommending people use ES modules to load our code until unpkg adds built-in support for minifying and rolling-up ES modules. That’d probably be the ideal from our perspective since then the module entry is just the source (but with nonstandard bare module specifiers for external dependencies), but web requests get minified bundles.

Copy link
Member

@mbostock mbostock Dec 9, 2018

Choose a reason for hiding this comment

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

@mbostock mbostock closed this in 0bd8f18 Dec 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants