asg017 / unofficial-observablehq-compiler

An unofficial compiler for Observable notebook syntax
https://www.npmjs.com/package/@alex.garcia/unofficial-observablehq-compiler
115 stars 22 forks source link

fix: Get imported modules before returning define function #11

Closed asg017 closed 4 years ago

asg017 commented 4 years ago

This PR does a few things, sorry for the big file changes:

  1. Style fixes - my code editor has builtin prettier formatting, so sorry about that... I wouldve added a .prettierrc but since the codebase is small, idk how effective it'd be
  2. cellsPromise -> createCellDefinition.
  3. cleaned up compile.notebook to use createModuleDefintion. Both functions were doing the same thing before.
  4. (biggest change) Dynamically import all the external modules before defining "regular" cells.

This change was needed because "regular" cells that depended on imported cells would error out saying importCell is not defined while the imported cells were being resolved. For example, say there was this module:

import {x} from '@user/big-notebook'

y = `x is ${x}`

So, the compiler would parse this module, then do a createCellDefintion on each cell. Creating the cell definition for x would take a long time - since it would have to import() the @user/big-notebook ES module, then it would finally call module.import(x, other). This is an async process.

For y, on the other hand, that would be defined before module.import(x, other} would be called, since regular cell definitions could be synchronous. So, module.define(y, ["x"], x => 'x is ${x}') would be executed, x wouldnt be defined yet, throwing the x is not defined error.

You could see this in our test.html pages - here's an example, notice how when I do a hard refresh, the RuntimeError: x is not defined error is throw for cells that depend on imported cells.

unofficial-observablehq-compiler-demo surge sh_test_test html

So, my solution is that before we return the async function define(runtime, observer) in createModuleDefintion, we first loop through all the import cells, resolve the modules that they depend on, then save that in a map. For example, dependencyMap could look like:

{
    "@asg017/notebook": async function define(){},
     "@user/module": async function define(){},
}

So now, when call createCellDefintion on all the cells, there's no need for import cells to by asynchronous anymore (since they can just call dependencyMap.get(name)), removing the error. Here's what the page looks like now, notice how there's no RuntimeError when I refresh:

54 186 152 31_3000_test_test html - Edited

Here are 2 side effects to this:

  1. All imported cells are imported by default, even if they, or their transitive outputs, are not used. However, this same behavior also occurs in Observable ESM define functions, like here: https://api.observablehq.com/@observablehq/introduction-to-imports.js?v=3 (since all the imports, define1-define7 are imported, no matter how they are used or not)
  2. compile.module and compile.notebook now return promises, making them async. I don't think this is a big deal, though

@bryangingechen would love to hear your thoughts on these changes! It (kindof) breaks the old compile.module and compile.notebook APIs, since they are now asynchronous, but I dont think that's too big of an issue. (and sorry for the large diff!)

github-actions[bot] commented 4 years ago

New demo site for this PR deployed to unofficial-observablehq-compiler-demo-11.surge.sh !

asg017 commented 4 years ago

To see the difference before/after with pre-fetching imported modules, open these two pages up side-by-side, and do hard refreshes on both and see when errors are thrown:

bryangingechen commented 4 years ago

Overall this looks great! I'd also suggest updating README.md.

bryangingechen commented 4 years ago

Oops, I hit submit on that comment before I was completely ready. Slightly longer thoughts:

github-actions[bot] commented 4 years ago

New demo site for this PR deployed to unofficial-observablehq-compiler-demo-11.surge.sh !