search

observablehq / stdlib

The Observable standard library.
https://observablehq.com/@observablehq/standard-library
ISC License
966 stars 83 forks source link

allow Library.require override #289

Closed mbostock closed 2 years ago

mbostock commented 2 years ago

This PR allows the user to change the require implementation by assigning to Library.require.

Originally, I intended this to be local to the Library instance (hence the resolve argument to the Library constructor). However, there are a number of places where we want to require stuff statically, such as within the AbstractFile class definition. Therefore it seems unavoidable to have only a single definition of require. (Eventually we want to move everything to dynamic ES module imports anyway…)

mbostock commented 2 years ago

Does this create the possibility of a race-condition if there are in-flight requires when setDefaultRequire is called?

You can’t really have multiple requires at the same time (because require is built around a single global function named define), so the expectation is that you set Library.require before you do anything else.

duaneatat commented 2 years ago

You can’t really have multiple requires at the same time (because require is built around a single global function named define)

Yep, that limitation is the reason I asked -- the race-condition I was referring to is if the user changed require while there are in-flight requires, since both requires depend on the global define. But I acknowledge that this is a pretty contrived scenario so this lgtm!

mbostock commented 2 years ago

This was totally broken… sorry for not testing before asking you to review. I fixed this in 6de65c31f2711cdaa105cfc7fd18f4c246805950. It’d be nice to have a better way to test this locally via unit tests.

mbostock commented 2 years ago

Also I needed to re-export requireFrom and resolveFrom in 784dcb3aca40a4c79d4cbb7cb7223630da38a7d8 so I could use them (without having to load d3-require twice, since d3-require is baked-in to this library).