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

(Auto)EmbedLiveSample #3973

Merged
merged 48 commits into from
Aug 16, 2021
Merged

(Auto)EmbedLiveSample #3973

merged 48 commits into from
Aug 16, 2021

Conversation

Gregoor
Copy link
Contributor

@Gregoor Gregoor commented Jun 4, 2021

This adds a fallback algorithm to how EmbedLiveSamples gather code to be included inside of the iframe. It is outlined in this issue by Will:
mdn/content#3548 (comment)

@peterbe
Copy link
Contributor

peterbe commented Jun 4, 2021

I'm not sure what this is. That code, which I'm not afraid of, is not something you eat for breakfast.
How do I test this? URLs? What should I keep an eye on? WHat's the motivation?

@Gregoor
Copy link
Contributor Author

Gregoor commented Jun 7, 2021

Forgot to create this as a draft until I have filled all the details in. It's something I started working on with Ryan and it is needed for the Markdown conversion. Will's comment over here is good context for how it should work: mdn/content#3548 (comment)

@Gregoor Gregoor marked this pull request as draft June 7, 2021 08:29
@peterbe
Copy link
Contributor

peterbe commented Jun 7, 2021

@Gregoor lemme know if there's any specific testing or code reviewing that you want me to look at.

@Gregoor
Copy link
Contributor Author

Gregoor commented Jun 8, 2021

I could use some help indeed, thanks Peter. The way @escattone and I implemented it is change the EmbedLiveSample rendering to operate on the renderedHTML, which is both more Markdown-proof and allows us to skip parsing and instead just use cheerio to find the embed. Now the problem is that we DO need the source location of the macro for flaws. Which makes me think we need to go back to reading the source and not the rendered HTML. Does anyone see a way around that?

@escattone
Copy link
Contributor

Haven't forgotten about this @Gregoor, but I don't think I'll get to it until tomorrow. Excited to take a look!

@@ -14,7 +14,7 @@
//
// See also : LiveSampleLink

var sampleId = $0;
var sampleId = $0 || $token.location.start.offset;
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️ I like how you decided to pass the token as part of the macro's execution context.

escattone and others added 2 commits June 16, 2021 15:32
Restrict the live-sample page builds to iframes referencing live-samples on the current page.
@escattone
Copy link
Contributor

escattone commented Jun 16, 2021

@Gregoor @peterbe The most recent test failures baffle me. I don't see them locally. When I run the build, everything looks good, and when I run the headless tests locally, they don't fail for me.

@Gregoor
Copy link
Contributor Author

Gregoor commented Jun 17, 2021

Okay from the fact that the amount of passing tests varies, pending on the timeout and that it passes on our local beefy machines, leads me to conclude that our tests are ACCIDENTAL BENCHMARKS. But I am not gonna blame them for being more than they set out to be but will try to rise to the challenge next week and do less slow stuff and see if I can make them pass that way.

@Gregoor
Copy link
Contributor Author

Gregoor commented Jun 22, 2021

I am going into the timeouting testing hypothesis again after all. I only found one straightforward speed-up, which was using generators. Now I'm going to see if I can even pass one of the failing tests pass with increased timeouts before I build up a whole narrative on that assumption.

@peterbe
Copy link
Contributor

peterbe commented Jul 7, 2021

Another way to trigger the crash is:

BUILD_FOLDERSEARCH=getting_started BUILD_FLAW_LEVELS="*:ignore, macros:warn" yarn build -l zh-tw

@peterbe
Copy link
Contributor

peterbe commented Jul 7, 2021

See http://localhost:3000/fr/docs/Learn/CSS/Styling_text/Fundamentals#_flaws
There's a

<p>{{ EmbedLiveSample('Style_de_fonte_graisse_transformation_et_décoration_de_texte', '100%', 220) }}</p>

there which we know gets messed up because of the é.
As a matter of fact, I know the problem can go away if you change it from:

<p>{{ EmbedLiveSample('Style_de_fonte_graisse_transformation_et_décoration_de_texte', '100%', 220) }}</p>

...to...

<p>{{ EmbedLiveSample('Style_de_fonte_graisse_transformation_et_decoration_de_texte', '100%', 220) }}</p>

(and do the same with the <h3 id="Style_de_fonte_graisse_transformation_et_decoration_de_texte">)

In fact, it's a nice feature we already have in main that it can at least give a useful hint to the writer what to do about it:
Screen Shot 2021-07-07 at 4 27 48 PM

This useful additional error message hint seems to be broken now :(

@Gregoor
Copy link
Contributor Author

Gregoor commented Jul 8, 2021

Thanks for the digging, Peter!

It's an honest mistake.
But what the heck is wrong with line 74?? return color.toString(16); What's that about?

That I just fixed, I was erroneously using the rendered HTML instead of the document's rawBody. Now that part looks good to me.

The ASCII thing I had not an idea about, but I'll focus my remaining time on getting the build command to run through for these translated-docs. So I think I might not get to solving the other thing!

@Gregoor
Copy link
Contributor Author

Gregoor commented Jul 8, 2021

Alrighty, that one is fixed. The only outstanding one should be the ASCII correction but I won't get to it anymore during my mozilla time. Pardon me!

@peterbe
Copy link
Contributor

peterbe commented Jul 8, 2021

Alrighty, that one is fixed. The only outstanding one should be the ASCII correction but I won't get to it anymore during my mozilla time. Pardon me!

What @escattone and I did was that we kinda gave up on the ASCII bugs. We couldn't solve it for various reasons because; reasons. That's why we desperately chickened out and just threw in a slightly more informative error message.
But now that the code is refreshed, perhaps we can take another stab at simply solving the bug entirely so you become allowed to use:

<h2 id="ëclair">Heading</h2>
{{EmbedLiveSample('ëclair')}}

What do you think Ryan?

build/index.js Outdated Show resolved Hide resolved
@shapkarin
Copy link
Contributor

@peterbe I'm not sure but that suggestion above can help with ASCII bug. At least it works at PR #4437 with russian language.

build/index.js Outdated Show resolved Hide resolved
escattone and others added 4 commits August 13, 2021 10:39
If the macro is "EmbedLiveSample", there are cases when the same call signatures yield different results. For example, when the live-sample ID provided as the first argument can't be found or is not provided at all, the result depends on the macro's location in the document.
@shapkarin deserves most of the credit for this fix
Copy link
Contributor

@peterbe peterbe left a comment

Choose a reason for hiding this comment

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

Just so we don't accidentally merge this just because CI is green.

TypeError: Cannot read property 'includes' of undefined
    at Node.<anonymous> (/Users/peterbe/dev/MOZILLA/MDN/yari/kumascript/src/live-sample.js:74:49)
    at /Users/peterbe/dev/MOZILLA/MDN/yari/node_modules/cheerio/lib/api/traversing.js:588:20
    at Array.filter (<anonymous>)
    at initialize.exports.filter (/Users/peterbe/dev/MOZILLA/MDN/yari/node_modules/cheerio/lib/api/traversing.js:634:18)
    at Object.buildLiveSamplePages (/Users/peterbe/dev/MOZILLA/MDN/yari/kumascript/src/live-sample.js:74:6)
    at buildDocument (/Users/peterbe/dev/MOZILLA/MDN/yari/build/index.js:329:38)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async buildDocumentInteractive (/Users/peterbe/dev/MOZILLA/MDN/yari/build/cli.js:50:29)
    at async buildDocuments (/Users/peterbe/dev/MOZILLA/MDN/yari/build/cli.js:130:9)
    at async Se._action (/Users/peterbe/dev/MOZILLA/MDN/yari/build/cli.js:343:60)

error: Cannot read property 'includes' of undefined

@peterbe
Copy link
Contributor

peterbe commented Aug 16, 2021

I found another error (after waiting a looooong time):

Error: Expected name, found ...of_loop
    at getName (/Users/peterbe/dev/MOZILLA/MDN/yari/node_modules/css-what/lib/parse.js:156:19)
    at parseSelector (/Users/peterbe/dev/MOZILLA/MDN/yari/node_modules/css-what/lib/parse.js:220:28)
    at Object.parse (/Users/peterbe/dev/MOZILLA/MDN/yari/node_modules/css-what/lib/parse.js:141:20)
    at Object.select (/Users/peterbe/dev/MOZILLA/MDN/yari/node_modules/cheerio-select/lib/index.js:155:50)
    at initialize.exports.find (/Users/peterbe/dev/MOZILLA/MDN/yari/node_modules/cheerio/lib/api/traversing.js:62:28)
    at initialize.module.exports (/Users/peterbe/dev/MOZILLA/MDN/yari/node_modules/cheerio/lib/cheerio.js:101:18)
    at new initialize (/Users/peterbe/dev/MOZILLA/MDN/yari/node_modules/cheerio/lib/load.js:41:20)
    at initialize (/Users/peterbe/dev/MOZILLA/MDN/yari/node_modules/cheerio/lib/load.js:38:14)
    at findSectionStart (/Users/peterbe/dev/MOZILLA/MDN/yari/kumascript/src/api/util.js:72:3)
    at hasHeading (/Users/peterbe/dev/MOZILLA/MDN/yari/kumascript/src/api/util.js:75:23)

error: Expected name, found ...of_loop

But note. I was hacking on testing a solution to ASCII-non-escaping and perhaps that broke things for the worse.

@escattone escattone merged commit 49f5317 into mdn:main Aug 16, 2021
@shapkarin
Copy link
Contributor

Congrats. It's important improvement. @escattone thanks to adding my code.

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.

4 participants