batfish icon indicating copy to clipboard operation
batfish copied to clipboard

Maybe expose Batfish helpers for client JS as a global variable instead of mystery modules

Open davidtheclark opened this issue 7 years ago • 5 comments

Batfish exposes some modules to client JS to help you work with the router (e.g. route-to) and utilize your configuration details (e.g. prefix-url).

Since these rely on the specifics of a given Batfish build (its config and routes), they are not actually regular JS modules. They are written as temporary files during the build, after your config and routes have been analyzed; then exposed via Webpack config to be imported. This means they only work when Webpack is running.

There are some unfortunate effects of this pattern:

  • If you wanted to look into the code you get when you import @mapbox/batfish/modules/route-to, you'd be surprised to find that that file does not exist in your node_modules directory.
  • If you use Flow or an ESLint plugin that checks imports, that tool will protest about this nonexistent file unless you add additional configuration.
  • Any other way you expose your code without a Webpack build step will break without additional configuration. For example, your tests against a file that imports route-to will have trouble with that import.

An alternative approach is to expose a global Batfish variable that contains the build-specific information. Then functions like routeTo and prefixUrl could:

  • EITHER be exposed on that object, e.g. Batfish.routeTo('/foo'), Batfish.prefixUrl('/foo');
  • OR be actual modules (that really exist in the file system) that accept a Batfish object as an argument, e.g. routeTo(Batfish, '/foo'), prefixUrl(Batfish, '/foo'), or else just expect one to be there and quietly fail if it isn't. (Looks like that last option is what Next.js does.)

A global variable doesn't bypass the problems listed above with non-Webpack tools consuming the JS. However, stubbing a global variable is much simpler in every case than stubbing a set of modules. And Batfish could expose a MockBatfish or something to help with tests.

@jfirebaugh I know you bumped into the weirdness of these Webpack-only imports in Batfish. I'm curious if you have any opinions about these alternatives, or have other ideas.

davidtheclark avatar Mar 04 '18 18:03 davidtheclark

This is maybe a way-too-ambitious proposal, but... would it be possible to expose these in a totally referentially transparent manner? E.g.:

batfish.config.js

import Batfish from 'batfish';
export default Batfish.config({
  siteBasePath: '/mapbox-gl-js',
  ...
});

pages/example.js

import Batfish from '../batfish.config';
export default class extends React.Component {
  render() {
    return <a href={Batfish.prefixUrl('/path')}>Path</a>
  }
}

jfirebaugh avatar Mar 07 '18 22:03 jfirebaugh

(Potentially confusing that Batfish is used to refer to two different things in different files there -- the library "namespace" in batfish.config.js and a fully-configured instance in the page. Open to a different suggested naming -- the point is that there's no magic or globals, just regular imports, exports, and function calls.)

jfirebaugh avatar Mar 07 '18 22:03 jfirebaugh

@jfirebaugh Interesting idea! I think the problem is that batfish.config.js is meant to run only in Node, but functions like prefixUrl run only in the browser. In batfish.config.js you'll want to use path and fs to access your file system, may want to utilize big libraries, etc.; but for the browser helpers we want the smallest possible footprint. We could make it look like you're importing the thing you exported from your batfish.config.js, but that would be trickery.

davidtheclark avatar Mar 08 '18 22:03 davidtheclark

Hm ... I've been jumping around a lot of different codebases lately, and after digging back into Batfish more in the past week I realized I misrepresented the situation in some ways.

  • The set of router-reliant modules exposed at @mapbox/batfish/modules/ are not in fact "virtual": they're real modules, the real file system. They're still a little weird because they're stateful: the functions are configured by Batfish's Router component when it mounts, then if you import and use them you get the configured behavior. Maybe that's fine.
  • I think the only purely "virtual" modules, then — i.e. modules with paths aliased by Webpack or Babel, which the user would have trouble finding in the file system — are:
    • When you inject data by adding dataSelectors to the config, the modules that you then import to get the selected data are virtual.
    • If you import @mapbox/batfish/modules/md in order to precompile Markdown within JS, that module doesn't actually exist. This is a detail of babel-plugin-jsxtreme-markdown: the import statement only exists to make the md template take look like its a real thing, in scope; but in fact it's all compiled away by Babel, and only makes sense to Babel.

cc @anandthakker

davidtheclark avatar Mar 14 '18 15:03 davidtheclark

@davidtheclark in the case of md, the babel precompilation sounds like an optimization, rather than an inherent requirement -- is that right? If so, could the Batfish package provide the md tag function as @mapbox/batfish/modules/md?

For the injected data, I like @jfirebaugh's idea of importing the data from the config (rather than from a special batfish module). Sure, as you mentioned above, this would still involve some trickery, but I think the trickery is farther behind the curtain and less likely to trip users up. (Especially if the batfish config looks like export default Batfish.config(...), which reads pretty clearly as "something that batfish returns after processing my config options")

anandthakker avatar Mar 14 '18 15:03 anandthakker