fix: allow also " inside of an embed #1598

luwol03 · 2021-06-22T12:35:15Z

Summary

I fixed a bug where ":include" is not handled like ':include'. This is not working because the type of quotes are not supported. The default prettier configuration changes the '' to "". And if you have auto format toggled on, you're not be able to save the file with the right quotes.

What kind of change does this PR introduce?

I added added the " also the the replace regex

str = str
      .replace(/^('|")/, '')
      .replace(/('|")$/, '')

For any code change,

Related documentation has been updated if needed
Related tests have been updated or tests have been added (I cant find some tests for that already.)

Does this PR introduce a breaking change? (check one)

Yes
No

If yes, please describe the impact and migration path for existing applications:

Related issue, if any:

closes #1597
closes #1587
closes #1555
closes #1005

Tested in the following browsers:

vercel · 2021-06-22T12:35:20Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/docsify-core/docsify-preview/BJiGEnyE33rV1Pzk3UTtre7AHZqb
✅ Preview: https://docsify-preview-git-fork-luwol03-issue-1597-118886-docsify-core.vercel.app

codesandbox-ci · 2021-06-22T12:37:39Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 633f241:

Sandbox	Source
docsify-template	Configuration
docsify-template (forked)	Issue #1597

sy-records

Can you add a test for this?

Koooooo-7

@sy-records
As we all know, this func getAndRemoveConfig has been used for much places.
It seems we need separate it for different functions in case of unpredicted issues.
WDTY?

sy-records · 2021-06-24T01:29:45Z

Yes, you're right.

trusktr · 2021-06-24T23:10:20Z

It seems we need separate it for different functions in case of unpredicted issues.

@Koooooo-7 What do you mean?

Koooooo-7 · 2021-06-25T01:25:33Z

It seems we need separate it for different functions in case of unpredicted issues.

@Koooooo-7 What do you mean?

This func r working in multi place to get configs. such as titles , sidebar headings.
If we add " for embed files, it might cause other problems in other places.
so, we may need separate this func , for sidebar, for titles and for embed files.

trusktr · 2021-06-25T02:52:27Z

@Koooooo-7 I see what you're saying, although it seems very unlikely. My guess would be that there is no one relying on he non-working double quotes.

It seems multiple people have hit this issue, and we closed the issues. It was in fact working before, then it broke:

Koooooo-7 · 2021-06-25T03:18:29Z

@trusktr I checked this func history, it seems never support the " before, the docs either.
You could check this func since Dec 5, 2018 and earlier.
If we support it right now, it would remove the " in other places also.
So, I prefer to separate this func.

luwol03 · 2021-06-25T14:40:23Z

Can you add a test for this?

sure, will try that.

It seems we need separate it for different functions in case of unpredicted issues.

@Koooooo-7 What do you mean?

This func r working in multi place to get configs. such as titles , sidebar headings.
If we add " for embed files, it might cause other problems in other places.
so, we may need separate this func , for sidebar, for titles and for embed files.

I dont think so. I would also like to use the double ones inside of headlines, or other stuff. Everywhere I can use this style.

And another question, do you prefere an clean commit history, so should I rebase merge upstream changes?

Koooooo-7 · 2021-06-25T15:19:47Z

@luwol03 of cuz we can support this feat for those configs. instead, how about those titles/other stuff without configs but with " ?
e.g a title this is "my title".

luwol03 · 2021-06-25T15:31:04Z

@luwol03 of cuz we can support this feat for those configs. instead, how about those titles/other stuff without configs but with " ?
e.g a title this is "my title".

Then we should improve the function, so that it only replaces the quotes, if they are followed by a :xxxx and there is also a closing one.

Koooooo-7 · 2021-06-25T15:40:14Z

@luwol03 of cuz we can support this feat for those configs. instead, how about those titles/other stuff without configs but with " ?
e.g a title this is "my title".

Then we should improve the function, so that it only replaces the quotes, if they are followed by a :xxxx and there is also a closing one.

agreed.
If possible, glad to see a good solution for this func/regx to solve those issues above.
I tested some cases locally again, it seems not break something （suppose I haven't missed something.）. I think it's okay for now.

luwol03 · 2021-06-25T17:47:37Z

Is this normal, that your e2e tests failed?

Koooooo-7 · 2021-06-25T17:50:51Z

Is this normal, that your e2e tests failed?

It seems not this changes broken, it's okay.
I would retry it.

trusktr · 2021-06-25T18:10:28Z

If we support it right now, it would remove the " in other places also.

@Koooooo-7 can you point them out for us?

ESIT: Oh NVM, i see you checked already and think it is fine.

trusktr · 2021-06-25T18:13:31Z

Is this normal, that your e2e tests failed?

Sometimes it fails for some reason despite that it shouldn't. Needs a fix.

luwol03 · 2021-06-25T19:03:00Z

Before merge, I will try that:

Then we should improve the function, so that it only replaces the quotes, if they are followed by a :xxxx and there is also a closing one.

trusktr · 2021-06-25T19:22:34Z

Before merge, I will try that:

Then we should improve the function, so that it only replaces the quotes, if they are followed by a :xxxx and there is also a closing one.

Is anyone using it without the : like "foo :bar"? Seems like it should be a space-separated set of anything.

trusktr · 2021-06-25T19:25:38Z

Here is an example without the colon: #1587 (comment)

~~Without a colon means to set a class I believe.~~ Nvm, i don't see a mention in the docs.

trusktr · 2021-06-25T19:35:25Z

test/unit/render-util.test.js

+        str: `[filename](_media/example.md ":include")`,
+      });
+    });
+  });


I wonder what the behavior should actually be. Seems funky that it leaves things behind. Seems like it should handle everything.

trusktr · 2021-06-25T20:17:33Z

@jhildenbiddle One test or other keeps randomly failing.

trusktr · 2021-06-25T20:21:05Z

Investigating the failures. Let's not merge until we know it isn't the change from here. Seems unlikely that it is.

trusktr

Looks good. I'm not requesting any changes, just blocking from merging to ensure the test is just flaky (that's what it seems like).

trusktr · 2021-06-25T22:22:00Z

I re-ran the test like 6 times already, and one (at random) still keeps failing.

trusktr · 2021-06-25T22:42:10Z

improve the function, so that it only replaces the quotes, if they are followed by a :xxxx and there is also a closing one.

I don't think we should do that, it could break this: "this-is-currently-ignored :but=this-currently-works". If we do, it can be a new PR.

Thanks @luwol03! Merging, since tests pass except one at random, which seems unrelated.

luwol03 · 2021-06-26T07:17:27Z

Oh, A few seconds ago, I pushed my changes, before I relized, that you already merged this changes 😂

Can you please take a look at my new commit? @trusktr Should I open a new PR for that?

sy-records · 2021-06-27T06:01:27Z

@luwol03 You can open a new PR

luwol03 · 2021-06-28T07:57:38Z

@luwol03 You can open a new PR
@sy-records done - see #1603

fix: allow also " inside of an embed

ec16e4a

vercel bot deployed to Preview June 22, 2021 12:35 View deployment

vercel bot deployed to Preview June 22, 2021 12:36 View deployment

sy-records reviewed Jun 23, 2021

View reviewed changes

Koooooo-7 self-requested a review June 23, 2021 11:35

Koooooo-7 reviewed Jun 23, 2021

View reviewed changes

This was referenced Jun 25, 2021

":include" in double quotes doesn't recognized #1587

Closed

':include' not working in Docsify 4.12.1 #1555

Closed

Embed files does not work #1005

Closed

Merge branch 'develop' into issue/1597-embed-quotes-bug

8f5cbad

vercel bot deployed to Preview June 25, 2021 09:57 View deployment

added tests

633f241

vercel bot deployed to Preview June 25, 2021 15:27 View deployment

Koooooo-7 approved these changes Jun 25, 2021

View reviewed changes

trusktr reviewed Jun 25, 2021

View reviewed changes

trusktr approved these changes Jun 25, 2021

View reviewed changes

trusktr requested changes Jun 25, 2021

View reviewed changes

trusktr approved these changes Jun 25, 2021

View reviewed changes

trusktr merged commit 74f17a0 into docsifyjs:develop Jun 25, 2021

trusktr mentioned this pull request Jun 25, 2021

flaky tests #1602

Closed

luwol03 mentioned this pull request Jun 28, 2021

improved getAndRemoveConfig util function #1603

Closed

9 tasks

adriangb mentioned this pull request Aug 3, 2021

Option to disable quote replacement executablebooks/mdformat#251

Closed

Koooooo-7 mentioned this pull request Jan 6, 2022

[Release]: v4.12.2 #1714

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: allow also " inside of an embed #1598

fix: allow also " inside of an embed #1598

luwol03 commented Jun 22, 2021 •

edited by trusktr

Loading

vercel bot commented Jun 22, 2021 •

edited

Loading

codesandbox-ci bot commented Jun 22, 2021 •

edited

Loading

sy-records left a comment

Koooooo-7 left a comment •

edited

Loading

sy-records commented Jun 24, 2021

trusktr commented Jun 24, 2021

Koooooo-7 commented Jun 25, 2021

trusktr commented Jun 25, 2021

Koooooo-7 commented Jun 25, 2021 •

edited

Loading

luwol03 commented Jun 25, 2021

Koooooo-7 commented Jun 25, 2021 •

edited

Loading

luwol03 commented Jun 25, 2021

Koooooo-7 commented Jun 25, 2021 •

edited

Loading

luwol03 commented Jun 25, 2021

Koooooo-7 commented Jun 25, 2021

trusktr commented Jun 25, 2021 •

edited

Loading

trusktr commented Jun 25, 2021

luwol03 commented Jun 25, 2021

trusktr commented Jun 25, 2021 •

edited

Loading

trusktr commented Jun 25, 2021 •

edited

Loading

trusktr Jun 25, 2021

trusktr commented Jun 25, 2021

trusktr commented Jun 25, 2021

trusktr left a comment

trusktr commented Jun 25, 2021

trusktr commented Jun 25, 2021

luwol03 commented Jun 26, 2021 •

edited

Loading

sy-records commented Jun 27, 2021

luwol03 commented Jun 28, 2021

fix: allow also " inside of an embed #1598

fix: allow also " inside of an embed #1598

Conversation

luwol03 commented Jun 22, 2021 • edited by trusktr Loading

Summary

What kind of change does this PR introduce?

For any code change,

Does this PR introduce a breaking change? (check one)

Related issue, if any:

Tested in the following browsers:

vercel bot commented Jun 22, 2021 • edited Loading

codesandbox-ci bot commented Jun 22, 2021 • edited Loading

sy-records left a comment

Choose a reason for hiding this comment

Koooooo-7 left a comment • edited Loading

Choose a reason for hiding this comment

sy-records commented Jun 24, 2021

trusktr commented Jun 24, 2021

Koooooo-7 commented Jun 25, 2021

trusktr commented Jun 25, 2021

Koooooo-7 commented Jun 25, 2021 • edited Loading

luwol03 commented Jun 25, 2021

Koooooo-7 commented Jun 25, 2021 • edited Loading

luwol03 commented Jun 25, 2021

Koooooo-7 commented Jun 25, 2021 • edited Loading

luwol03 commented Jun 25, 2021

Koooooo-7 commented Jun 25, 2021

trusktr commented Jun 25, 2021 • edited Loading

trusktr commented Jun 25, 2021

luwol03 commented Jun 25, 2021

trusktr commented Jun 25, 2021 • edited Loading

trusktr commented Jun 25, 2021 • edited Loading

trusktr Jun 25, 2021

Choose a reason for hiding this comment

trusktr commented Jun 25, 2021

trusktr commented Jun 25, 2021

trusktr left a comment

Choose a reason for hiding this comment

trusktr commented Jun 25, 2021

trusktr commented Jun 25, 2021

luwol03 commented Jun 26, 2021 • edited Loading

sy-records commented Jun 27, 2021

luwol03 commented Jun 28, 2021

luwol03 commented Jun 22, 2021 •

edited by trusktr

Loading

vercel bot commented Jun 22, 2021 •

edited

Loading

codesandbox-ci bot commented Jun 22, 2021 •

edited

Loading

Koooooo-7 left a comment •

edited

Loading

Koooooo-7 commented Jun 25, 2021 •

edited

Loading

Koooooo-7 commented Jun 25, 2021 •

edited

Loading

Koooooo-7 commented Jun 25, 2021 •

edited

Loading

trusktr commented Jun 25, 2021 •

edited

Loading

trusktr commented Jun 25, 2021 •

edited

Loading

trusktr commented Jun 25, 2021 •

edited

Loading

luwol03 commented Jun 26, 2021 •

edited

Loading