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

Either enable emoji plugin or not #1726

Closed

Conversation

ChoKaPeek
Copy link
Contributor

@ChoKaPeek ChoKaPeek commented Jan 17, 2022

Summary

Disclaimer: All the issues I have referenced were directly affected by the placeholder function. I am not pushing for emoji parsing to be disabled by default, but there are big issues with the actual placeholder / three-way system. It might be nice to add the emoji plugin to the default html

  • Remove the placeholder emojify function dealing more bad than good.
    • replaces any non-empty couple of colons
    • makes the user think emojis are enabled
    • duplicates management of the same problem
    • one uses the unicode value, the other the word
    • pushes the user to use __colon__ or : "quick-and-dirty" fixes
  • Remove the noEmoji configuration option that can unintuitively be set at the same time as the emoji plugin.
  • Update documentation.
  • Keep replacing __colon__ to : to stay compatible with most quick-and-dirty fixes out there.

This does not seem to need a three-way gate. As long as the plugin is installed, emojis are enabled. Otherwise they aren't. Saves time, avoids confusion and configuration, skips some computation.

If I am not wasting my time, ping me so I can fix tests.

What kind of change does this PR introduce?

Feature

For any code change,

  • Related documentation has been updated if needed
  • Related tests have been updated or tests have been added

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

  • Yes
  • No

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

  • install the emoji plugin, if needed, if not already done
  • remove the now implicit noEmoji option [optional]

Related issue, if any:

#1721
#1380
#853
#742
and so on

Tested in the following browsers:

  • Chrome
  • Firefox
  • Safari
  • Edge
  • IE

@vercel
Copy link

vercel bot commented Jan 17, 2022

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/EcPr6PCNf3SUVXVgFNStQKGnR6FN
✅ Preview: https://docsify-preview-git-fork-chokapeek-feat-no-6b4e3b-docsify-core.vercel.app

@codesandbox-ci
Copy link

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 a52946f:

Sandbox Source
docsify-template Configuration

@trusktr
Copy link
Member

trusktr commented Jan 23, 2022

Hello @ChoKaPeek ! Thanks for submitting this. I will circle back soon.

Copy link
Member

@trusktr trusktr left a comment

Choose a reason for hiding this comment

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

Personally I like this change, but it is a breaking change. This would require us to:

  • bump to v5
  • create the new migration log we've been chatting about, and describe the step to migrate from the previous version to the new one
  • there are a high number of people not using version numbers in script tags that load Docsify from script tag. We need to either
    • keep the v4 docsify{.min}.js in the same place, and output a new v5 script to a new location in the output folder, that way people can opt into the new version
    • or just let people's emojies break, and when they ask, we point them to migration log.

Ultimately, for health of the project, I think breaking emoji's is better for the long term, but I can understand if we go with the non-breaking approach.

Related is PR #1733 (prevent people from starting Docsify sites with version-less links)

@trusktr
Copy link
Member

trusktr commented Jan 29, 2022

Here's a shorter way to put it:

I think people making stable sites by default (stable as in it works exactly how they tested it) is better as a default. People can use @X instead of @X.X.X is they wish to opt into automatic updates. But IMO better if people update their versions when they feel is a good time rather than us doing it.

I've never written a web app where it automatically updates itself and publishes itself with new dependencies. In my opinion, that is a recipe for periodical breakage, and more periodical work (from years of experience).

Do you write websites that auto-update themselves to latest in-range versions of dependencies?

I can't imagine doing this for a client, being done with a job, and them coming back to me asking why their site is broken every so often.

It's not a good idea.

@jhildenbiddle
Copy link
Member

Agreed that both the use of the emoji.js plugin and the emoji parsing code need some attention.

See #779 for proposals on rending emoji and the the use of the emoji plugin moving forward. Much of what you've outlined here is covered there.

In the meantime, there are simple changes that can be made to the emoji parser to address many of the issues you've linked to here. I'm going to take look at what non-breaking changes can be made short-term, then I'll circle back.

@jhildenbiddle
Copy link
Member

jhildenbiddle commented Feb 6, 2022

These issues have been addressed as a non-breaking change with #1746.

As for version locks in URLs, this is a separate topic that has been covered at great length elsewhere (#1733 being one of several places).

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.

None yet

3 participants