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

Light bundle version #14

Merged
merged 19 commits into from
Feb 19, 2021
Merged

Light bundle version #14

merged 19 commits into from
Feb 19, 2021

Conversation

arturcarvalho
Copy link
Contributor

@arturcarvalho arturcarvalho commented Feb 16, 2021

Hi again @wooorm.

This PR is related to this discussion

First of all, thanks! I've reduced the bundle chunk by about 200kB!

What I did:

  1. Copied the rehype-highlight index.js to my repo and changed the require to var lowlight = require('lowlight/lib/core');
  2. Went from remark-html to my own remark-rehype and rehype-stringify.
  3. Instead of remark-highlight I used my own remark-highlight as said on the first point.

There's a strange thing going on, if I import the lib/languages instead of require them, the bundle size stays big. I'm using nextjs, maybe it's related.

This is the calling file:

import slug from 'remark-slug';
import headings from 'remark-autolink-headings';
import hint from 'remark-hint';
import gfm from 'remark-gfm';
import remark from 'remark';
import m2h from 'remark-rehype';
import html from 'rehype-stringify';
import highlight from './rehypelight';

export async function markdownToHtml(markdown: string) {
  const result = await remark()
    .use(gfm) // underline, table, etc.
    .use(slug) // creates slugs on headings
    .use(headings, { behavior: 'wrap' }) // wrap puts it after slug, adds url
    .use(hint)
    .use(m2h)
    .use(highlight, {
      languages: {
        // requires are needed, imports don't reduce the bundle size.
        javascript: require('highlight.js/lib/languages/javascript'),
        typescript: require('highlight.js/lib/languages/typescript'),
      },
    })
    .use(html)
    .process(markdown);

  return result.toString();
}

Now to the PR (and its problems):

  • I've created the core file and moved the logic there. The tests are OK.
  • I've started making some tests pass for the light version.

The issues:

  1. I don't know how to add another export. Is it on the files field? My require/npm publishing experience is limited.
  2. I don't know how to test if the bundles are smaller.
  3. Should I just duplicate the tests? I didn't have time to understand them all. I'll move them to the main test.js once they work. I'd prefer to go one by one from a clean slate in a different file for starters.
  4. How should at least one language be enforced and which options field to use? I've used the languages field.

In any case, thanks for your help and hope this is helpful!

@codecov-io

This comment has been minimized.

Copy link
Member

@wooorm wooorm left a comment

Choose a reason for hiding this comment

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

Nice work, thanks!

I don't know how to test if the bundles are smaller.

I don’t think that’s needed to add. This should work.
But just to be sure, you could take a look at the npm script build-mangle, and use similar code to see if it actually works (I like using https://github.com/sindresorhus/gzip-size-cli to check the sizes of files)

How should at least one language be enforced and which options field to use? I've used the languages field.

For testing? How do you mean?


There should be some docs on this new plugin. Similar to the Browser section in lowlight (https://github.com/wooorm/lowlight#browser) perhaps?

core.js Outdated Show resolved Hide resolved
core.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
light.js Outdated Show resolved Hide resolved
light.js Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
test-light.js Outdated
'<pre><code class="hljs language-javascript"><span class="hljs-meta">"use strict"</span>;</code></pre>'
].join('\n'),
'should highlight (no class)'
)
Copy link
Member

Choose a reason for hiding this comment

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

I think these test can go in the main test file, no need to copy-paste it all!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How should at least one language be enforced and which options field to use? I've used the languages field.

For testing? How do you mean?

If a user doesn't provide a language, there won't be any highlighting. Should it be mandatory that the user provides at least one language? Is the field "languages" a good place to put the selected languages to bundle?

There should be some docs on this new plugin. Similar to the Browser section in lowlight (https://github.com/wooorm/lowlight#browser) perhaps?

I'll get to it at the end if it's OK with you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think these test can go in the main test file, no need to copy-paste it all!

I suppose this is related to my question:
Should I just duplicate the tests? I didn't have time to understand them all. I'll move them to the main test.js once they work. I'd prefer to go one by one from a clean slate in a different file for starters.

Copy link
Member

Choose a reason for hiding this comment

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

yes, for the tests, indeed!

Copy link
Member

Choose a reason for hiding this comment

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

If a user doesn't provide a language, there won't be any highlighting. Should it be mandatory that the user provides at least one language? Is the field "languages" a good place to put the selected languages to bundle?

I think that should be clearly documented. I don’t think we need to do extra work to go into the languages object to see if there’s at least one key or not 🤷‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If a user doesn't provide a language, there won't be any highlighting. Should it be mandatory that the user provides at least one language? Is the field "languages" a good place to put the selected languages to bundle?

I think that should be clearly documented. I don’t think we need to do extra work to go into the languages object to see if there’s at least one key or not 🤷‍♂️

"no extra work " seems fine with me 😁

arturcarvalho and others added 10 commits February 17, 2021 10:52
Co-authored-by: Titus <tituswormer@gmail.com>
Co-authored-by: Titus <tituswormer@gmail.com>
Co-authored-by: Titus <tituswormer@gmail.com>
Co-authored-by: Titus <tituswormer@gmail.com>
Co-authored-by: Titus <tituswormer@gmail.com>
Co-authored-by: Titus <tituswormer@gmail.com>
Co-authored-by: Titus <tituswormer@gmail.com>
@arturcarvalho arturcarvalho marked this pull request as ready for review February 17, 2021 20:08
test.js Outdated Show resolved Hide resolved
Copy link
Member

@wooorm wooorm left a comment

Choose a reason for hiding this comment

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

nice!

A couple of nits on the docs (especially the example, I placed it in a file locally, formatted it, so that it matches the coding style of this project)

I don’t think you need so many tests! Most of the code is shared with the main plugin, so there’s no need to repeat every test! 3-5 would be fine by me

readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
test.js Outdated
Comment on lines 442 to 447
var light = require('./light')
var js = require('highlight.js/lib/languages/javascript')
var as = require('highlight.js/lib/languages/applescript')
var cp = require('highlight.js/lib/languages/cpp')
var lt = require('highlight.js/lib/languages/latex')
var su = require('highlight.js/lib/languages/subunit')
Copy link
Member

Choose a reason for hiding this comment

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

please move these to the top, so when opening this file it’s immediately clear what’s loaded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be good.

test.js Outdated Show resolved Hide resolved
arturcarvalho and others added 5 commits February 18, 2021 09:20
Co-authored-by: Titus <tituswormer@gmail.com>
Co-authored-by: Titus <tituswormer@gmail.com>
Co-authored-by: Titus <tituswormer@gmail.com>
Co-authored-by: Titus <tituswormer@gmail.com>
@wooorm wooorm merged commit bf464d7 into rehypejs:main Feb 19, 2021
@wooorm wooorm added ⛵️ status/released 🗄 area/interface This affects the public interface 🦋 type/enhancement This is great to have 🧒 semver/minor This is backwards-compatible change labels Feb 19, 2021
@wooorm
Copy link
Member

wooorm commented Feb 19, 2021

awesome, thanks for your hard work on this @arturcarvalho. Released!

@arturcarvalho
Copy link
Contributor Author

awesome, thanks for your hard work on this @arturcarvalho. Released!

🎉 Thank you, it was a pleasure working with you.

Really liked the process around it all. Practical but efficient and thorough. Even the apostrophes are being checked 😁

Look at this beauty (bottom line):
Before:
image

After:
image

@arturcarvalho arturcarvalho deleted the patch-1 branch February 22, 2021 09:47
@wooorm wooorm added the 💪 phase/solved Post is done label Aug 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗄 area/interface This affects the public interface 💪 phase/solved Post is done 🧒 semver/minor This is backwards-compatible change 🦋 type/enhancement This is great to have
Development

Successfully merging this pull request may close these issues.

None yet

3 participants