-
Notifications
You must be signed in to change notification settings - Fork 130
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
import FileAttachment #325
Conversation
f83cf7f
to
87df0cf
Compare
87df0cf
to
7e8e722
Compare
@@ -2,7 +2,13 @@ | |||
|
|||
TK Should this be called “working with data”? | |||
|
|||
You can load files the built-in `FileAttachment` function or the standard [`fetch`](https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch) API. We recommend `FileAttachment` because it supports many common data formats, including CSV, TSV, JSON, Apache Arrow, and SQLite. For example, here’s how to load a CSV file: | |||
You can load files the built-in `FileAttachment` function. This is available by default in Markdown, but you can import it like so: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can load files the built-in `FileAttachment` function. This is available by default in Markdown, but you can import it like so: | |
You can load files with the built-in `FileAttachment` function. This is available by default in Markdown, but you can import it like so: |
going to continue reviewing tomorrow
Nice! Is there any chance that this could also work in data loaders? This way a loader (say, one that does analysis in ts) could use the product of another loader (one that downloads a dataset). Currently the import fails with ERR_UNSUPPORTED_ESM_URL_SCHEME, which is a generic problem already discussed in #288. |
Maybe @Fil. I think I had a Slack thread where I showed how to get the npm: protocol imports working in a data loader. But getting the import working is sort of beside the point — the API for chaining loaders isn’t really about Future work though. Is there an issue already for chaining loaders? |
Now there is one: #332 |
@@ -20,9 +18,11 @@ export interface DatabaseReference { | |||
} | |||
|
|||
export interface FileReference { | |||
/** The relative path from the source root to the file. */ | |||
name: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the name is still the relative path from the page to the file?
docs/
├── earthquakes.csv
└── sub
└── index.md
and in docs/sub/index.md
we have FileAttachment("../earthquakes.csv")
, name is "../earthquakes.csv"
vs "./earthquakes.csv"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can double check but there is a difference between Feature.name, FileAttachment.name in generated code, and FileReference.name. It’s all a bit hard to keep straight.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right! 👍 Sorry, I was confused. I’ve updated the comment.
const {imports, fetches} = findImports(body, root, sourcePath); | ||
const features = findFeatures(body, root, sourcePath, references, input); | ||
const references = findReferences(body); | ||
findAssignments(body, references, input); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have good test coverage for nvm there are comprehensive test cases at the endtranspileJavaScript
but it would be nice to have specific unit tests for the find<...>
functions. I can work that if it's helpful
@@ -28,16 +28,173 @@ describe("parseLocalImports(root, paths)", () => { | |||
}); | |||
it("ignores missing files", () => { | |||
assert.deepStrictEqual( | |||
parseLocalImports("test/input/imports", ["static-import.js", "does-not-exist.js"]).imports.sort(compareImport), | |||
parseLocalImports("test/input/imports", ["static-import.js", "does-not-exist.js"]).imports.sort(order), | |||
[ | |||
{name: "bar.js", type: "local"}, | |||
{name: "does-not-exist.js", type: "local"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused by this test case - is {name: "does-not-exist.js", type: "local"}
supposed to be included?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we don’t check for existence, just a valid path.
This allows you to import and use
FileAttachment
from ES modules. For example:Under the hood, this works by rewriting the call to
FileAttachment
to pass inimport.meta.url
; this allows theFileAttachment
constructor to resolve the path to the file relative to the importing module in the same fashion asimport
.Since you can use
FileAttachment
in ES modules now, I’ve also removed the rewriting offetch
;fetch
is now unadulterated, and calls tofetch
won’t be statically analyzed and turned into file attachments. The motivation here is to encourage people to standardize onFileAttachment
sinceFileAttachment
enforces static analysis; unlikefetch
,FileAttachment
throws a syntax error if you pass arguments that are not statically analyzable. In addition, while we can rewrite direct calls tofetch
, we can’t rewrite indirect calls tofetch
such asd3.json
andd3.csv
, so it’s better to be more explicit and limit automatic resolution toFileAttachment
.Fixes #15.