Skip to content
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

note loaders #1776

Merged
merged 17 commits into from
Nov 6, 2024
Merged

note loaders #1776

merged 17 commits into from
Nov 6, 2024

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Oct 22, 2024

  • get source & params for pages
  • get source & params for modules
  • get source & params for files (copied and generated)
  • write the build manifest
  • fix types in deploy
  • test deploy
  • add unit test for all 4 cases
  • add config:{root} to the build manifest
  • fix deploys (currently error 400)
  • add unit test for deploys

src/loader.ts Show resolved Hide resolved
@Fil Fil requested a review from mythmon October 23, 2024 18:09
src/build.ts Outdated Show resolved Hide resolved
@mythmon
Copy link
Member

mythmon commented Oct 28, 2024

I'm not sure I'm following what this is doing exactly. I used this to build the Framework docs, and the generated _build.json had lots of entries that didn't seem to exist in dist/. For example, /observable.png was listed in the files key, but dist/observable.png doesn't exist. It's actually at dist/_file/observable.1af93621.png.

I expected this manifest would only list items when they were a part of the dynamicPaths option (or a similar one if we rename this). I also expected that the path field would be the path to the file in the dist directory, and thus the path on the esrver. Did I misunderstand? Or is this just not quite done yet because it is in draft?

Fil added 2 commits October 29, 2024 09:36
fix manifest for non-exported files and modules (but commented out)
@Fil
Copy link
Contributor Author

Fil commented Oct 29, 2024

Thanks for the review. I have now:

  • fixed the file name for normal (non-exported) files and modules, and
  • commented them out of the manifest, leaving only the files and modules found in dynamicPaths

Fixing code to comment it out is a bit of a contradiction. It is because, even if it's out of scope for now, I think there is value potentially in knowing which (non-exported) files and modules depend on which source files. So maybe it would be right to uncomment these two calls, even if we don't yet have a reporter script to make use of them. (Or at least we know that we can reinstate them when we want.)

For instance, for an internal (non exported) parameterized module, it's nice to know that _import/module-exported.281d2524.js is coming from /module-[type].js:

{
  "params": {"type": "exported"},
  "path": "/_import/module-exported.281d2524.js",
  "source": "/module-[type].js"
}

@mythmon
Copy link
Member

mythmon commented Oct 29, 2024

I agree that information would be interesting. I think that I'd want to be able to distinguish the unexported items from the exported ones though. Maybe with a boolean flag?

@Fil
Copy link
Contributor Author

Fil commented Oct 29, 2024

Added the source information for non-exported modules and files — with new keys (_file and _import) rather than a boolean. (removed after review)

@Fil
Copy link
Contributor Author

Fil commented Nov 1, 2024

tested live… 🎉

@Fil Fil marked this pull request as ready for review November 1, 2024 16:28
Copy link
Member

@mythmon mythmon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies, I didn't realize this was waiting on me.

src/build.ts Outdated
files: []
};
const addToManifest = (type: string, file: string, {title, path}: {title?: string | null; path?: string}) => {
const source = path == null || path === file.slice(1) ? null : join("/", path);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is path == null || path === file.slice(1) testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

path === file.slice(1) is for files that point to themselves (e.g. path=chart.js file=/chart.js)

the path is null for pages that have no source
this is only used in the tests for parseMarkdown… I can find a better way to express this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit better now; I'm still a bit unhappy with the naming (too many things we call "path")

@Fil Fil merged commit 5fd5703 into main Nov 6, 2024
4 checks passed
@Fil Fil deleted the fil/dag branch November 6, 2024 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants