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

Decouple linkify in separate plugin #351

Merged
merged 2 commits into from
Jul 24, 2019
Merged

Decouple linkify in separate plugin #351

merged 2 commits into from
Jul 24, 2019

Conversation

TrySound
Copy link
Collaborator

Closes #343

linkify depends on autolinker which is quite heavy. The option is
disabled by default and not used most of the time.

https://bundlephobia.com/result?p=autolinker@3.1.0

In this diff I decoupled linkify into plugin which can be accessed like
this

import Remarkable from 'remarkable';
import linkify from 'remarkable/linkify';

const md = new Remarkable({ html: true })
  .use(linkify);

For UMD build linkify provided out of the box

const md = new window.Remarkable({ html: true })
  .use(window.Remarkable.linkify);

Closes #343

linkify depends on autolinker which is quite heavy. The option is
disabled by default and not used most of the time.

https://bundlephobia.com/result?p=autolinker@3.1.0

In this diff I decoupled linkify into plugin which can be accessed like
this

```js
import Remarkable from 'remarkable';
import linkify from 'remarkable/linkify';

const md = new Remarkable({ html: true })
  .use(linkify);
```

For UMD build linkify provided out of the box

```js
const md = new window.Remarkable({ html: true })
  .use(window.Remarkable.linkify);
```
@TrySound TrySound force-pushed the decouple-autolinker branch from 5b1cb57 to d887353 Compare July 23, 2019 14:00
@coveralls
Copy link

Coverage Status

Coverage increased (+0.0007%) to 98.584% when pulling d887353 on decouple-autolinker into 19c515a on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.0007%) to 98.584% when pulling d887353 on decouple-autolinker into 19c515a on master.

@coveralls
Copy link

coveralls commented Jul 23, 2019

Coverage Status

Coverage increased (+0.002%) to 98.586% when pulling 766f50a on decouple-autolinker into 19c515a on master.

@TrySound
Copy link
Collaborator Author

@jonschlinkert @doowb @shockey are you okay with this change?

@doowb
Copy link
Collaborator

doowb commented Jul 24, 2019

I like the change, it looks great!

I'm assuming the reason for doing it this way is because it gives bundlers a chance to tree shake and exclude autolinker from the bundle when linkify is not used.

I think this change should wait until a major version bump is ready. Even though the default for linkify is false, any users that have linkify set to true will have different behavior and will need to upgrade.

@TrySound
Copy link
Collaborator Author

I can add warning that linkify option is replaced with plugin. Major bump is almost ready (at least from my POV). This PR is a blocker for #350.

@doowb
Copy link
Collaborator

doowb commented Jul 24, 2019

In #350 you mentioned: internals are hidden and changing them will not affect users. I assumed that once the changes are made and the code is bundled that version 1.7.2 could be published . Then 2.0.0 could be published with this PR (and any others that might have a breaking change).

If you're saying that all of the other stuff would require a major bump, then I'm fine with this PR being merged now. Especially because I'm excited to see all the work you've been doing get released 😀 , but I'm still wary about breaking changes in a patch bump.

If you have a plan for the releases, I'm good with that and I'll approve my review.

@TrySound
Copy link
Collaborator Author

I think removed bower support is a breaking change so we cannot publish 1.7.2.

Copy link
Collaborator

@shockey shockey left a comment

Choose a reason for hiding this comment

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

Looks good to me as a major version bump

@TrySound TrySound merged commit d627e16 into master Jul 24, 2019
@TrySound TrySound deleted the decouple-autolinker branch July 24, 2019 19:53
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.

Decouple autolinker dependency
4 participants