Skip to content

Commit

Permalink
[FE-10899] Fix old Observable imports (#181)
Browse files Browse the repository at this point in the history 
* Remove 'umd' target

* Remove unpkg / jsdelivr declarations

* Remove noop externals
  • Loading branch information
@jonvuri
jonvuri committed Apr 15, 2020
1 parent b3dfe2a commit 9557f33
Show file tree
Hide file tree
Showing 3 changed files with 18,015 additions and 368 deletions.
18,369 changes: 18,015 additions & 354 deletions dist/browser-connector.js
View file

Large diffs are not rendered by default.

6 changes: 0 additions & 6 deletions src/noop.js
View file

This file was deleted.

8 changes: 0 additions & 8 deletions webpack.browser.js
View file
Expand Up @@ -16,13 +16,6 @@ module.exports = {
{test: /\.js$/, loader: "babel-loader", exclude: /node_modules/}
]
},
externals: {
thrift: "../src/noop.js",
url: "../src/noop.js",
"../build/thrift/node/omnisci_types.js": "../src/noop.js",
"../build/thrift/node/common_types.js": "../src/noop.js",
"../build/thrift/node/OmniSci.js": "../src/noop.js",
},
node: {
child_process: "empty",
fs: "empty",
Expand All @@ -31,7 +24,6 @@ module.exports = {
},
output: {
path: path.join(__dirname, "dist"),
libraryTarget: 'umd',

This comment has been minimized.

Sign in to view

Copy link
@domoritz

domoritz Apr 20, 2020

Contributor

I think this change is breaking my rollup builds in Falcon. What's the reason for not building an umd package anymore? #181 doesn't really explain it.

This comment has been minimized.

Sign in to view

Copy link
@jonvuri

jonvuri Apr 20, 2020

Author Contributor

@domoritz Mostly, a manual regression of https://github.com/omnisci/mapd-connector/pull/148/files, which broke rawgit Observable imports as described in #181. Would love to find a fix that involves keeping umd packaging too, but we didn't have capacity to look into that for this release.

If you can confirm a fix for both umd and Observable imports, would love to review a PR for it!

You could also tag to a prior version for now, to see if it works for Falcon. We haven't made any breaking changes, so unless you're using new APIs it should continue to work.

This comment has been minimized.

Sign in to view

Copy link
@domoritz

domoritz Apr 20, 2020 •
edited

Contributor

Yep, I pinned the version but would love to stay up to date.

MapdCon = require("https://rawgit.com/mapd/mapd-connector/master/dist/browser-connector.js")
	.catch(() => new window.MapdCon())

looks like a hack to me. Ideally, this would to be needed in the first place. The import should just be

MapdCon = require("mapd-connector)

or

mapd = require("mapd-connector)
mapd.MapCon()

no?

Why don't you support normal named exports that work in both bundlers and observable? See for example https://github.com/vega/vega-lite/blob/master/rollup.config.js#L23. Vega-Lite works in Observable, browsers, and with bundlers (webpack, rollup, parcel, ...).

This comment has been minimized.

Sign in to view

Copy link
@jonvuri

jonvuri Apr 21, 2020

Author Contributor 
MapdCon = require("https://rawgit.com/mapd/mapd-connector/master/dist/browser-connector.js")
	.catch(() => new window.MapdCon())

^^ This is absolutely a hack (one recommended by Observable in things like https://observablehq.com/@observablehq/module-require-debugger, but nonetheless a hack for modules that don't really export correctly like ours). The issue is that hack - and the pinning to latest master - is what's already used across several Observable notebooks already, including ones we don't control.

We could break them in the name of progress, but it might be more opportune for us to make the big changes when we do the name change anyway, and leave mapd-connector supporting these older cases as long as possible.

This comment has been minimized.

Sign in to view

Copy link
@domoritz

domoritz Apr 21, 2020 •
edited

Contributor

Hmm, I think if someone depends on master, you cannot really guarantee anything but that's my opinion. Maybe you can send change requests to the ones you don't control (looks like there are only a handful of examples: https://observablehq.com/search?query=mapd-connector).

Didn't you make the change to UMD back in October? I'm still not understanding why the switch away from UMD happened now.

I think it would be nice for mapd to support named imports since I think it would allow people to use mapd outside observable and also make the observable experience with mapd better.
filename: "browser-connector.js"
}
}

0 comments on commit 9557f33

Please sign in to comment.