Skip to content

fix client error when the target is missing (e.g. it's in a comment) #378

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

Closed
wants to merge 3 commits into from

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Dec 13, 2023

Works by taking the cell out of the dataflow: it won't react to its inputs anymore, won't run, and won't register files and databases.

For an example imagine that at the bottom of docs/lib/d3.md you have the following nasty code with side effects:

${(console.warn("ahahah"), svg.remove())}

If you comment it out, you probably don't want the chart to be removed, nor the warning to appear on the console:

<!--
${(console.warn("ahahah"), svg.remove())}
-->

What this PR does not do is what happens server-side: i.e. if the comment registers a FileAttachment, the file will still be built. Could be a future enhancement?

closes #375

…ip it.

As a consequence its value is not part of the dataflow anymore, which is probably what the user wanted.

For an example, add the following to the bottom of docs/lib/d3.md:

```html
<!--
${(console.warn("ahahah"), svg.remove())}
-->
```
@Fil Fil requested review from mbostock and cinxmo December 13, 2023 14:03
@cinxmo
Copy link
Contributor

cinxmo commented Dec 15, 2023

What this PR does not do is what happens server-side: i.e. if the comment registers a FileAttachment, the file will still be built. Could be a future enhancement?

Is the file still built? I don't see a network call:

```js
import {FileAttachment} from "npm:@observablehq/stdlib";
```
<!-- <div>${await FileAttachment("gistemp.csv").csv({typed: true})}</div> -->

@Fil
Copy link
Contributor Author

Fil commented Dec 15, 2023

It's not requested by the page, but I think it's still saved to dist/_file/ by the build script?

@mbostock
Copy link
Member

I think it might be better to change how we parse, so that a commented-out inline expression isn’t evaluated at all, i.e., it’s treated as part of the comment text rather than running. Feels related to the other parsing weaknesses we’ve seen #396 #32 #11. On the other hand the solution here is simple and practical, so could be worth including. 🤔

@Fil
Copy link
Contributor Author

Fil commented Dec 18, 2023

I didn't find how to do this by parsing, but I'm not too familiar with the parser.

@mbostock
Copy link
Member

Yes, I think we will probably need a major overhaul of the parser rather than our current approach.

@Fil
Copy link
Contributor Author

Fil commented Feb 2, 2024

superseded by #597

@Fil Fil closed this Feb 2, 2024
@Fil Fil deleted the fil/fix-375 branch February 2, 2024 09:55
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.

uncaught TypeError: HTML comments with template literals and string interpolation
3 participants