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

"Tree-Shaking" support for TipTap #16

Closed
PhouvanhKCSV opened this issue Sep 6, 2018 · 17 comments · Fixed by #62
Closed

"Tree-Shaking" support for TipTap #16

PhouvanhKCSV opened this issue Sep 6, 2018 · 17 comments · Fixed by #62

Comments

@PhouvanhKCSV
Copy link
Contributor

It look like "tiptap-extensions" is load all extensions even i did not use all of them.

import {
    BlockquoteNode,
    BulletListNode,
    HardBreakNode,
    HeadingNode,
    ImageNode,
    ListItemNode,
    OrderedListNode,
    BoldMark,
    ItalicMark,
    HistoryExtension,
  } from 'tiptap-extensions'

I use only these extensions but in my vendor file, there is "highlight.js" dependency.

It would be nice if we can import only what we use.

@philippkuehn
Copy link
Contributor

Oh. This is bad. I don't know how to solve this right now. 😕

@dannyrb
Copy link

dannyrb commented Sep 6, 2018

I mean, you don't tree shake, you just "support" tree shaking. An example would be... WebPack goes to bundle your code, and it places comments around the modules that weren't used. Usually putting a comment before the unused module like:

/* unused harmony export <export name> */

Then a minifier, like uglify, removes the unused code during the minification process. You can read a bit more on it here: https://webpack.js.org/guides/tree-shaking/

See the requirements section:

  • Use ES2015 module syntax (i.e. import and export).
  • Add a "sideEffects" property to your project's package.json file.
  • Use production mode configuration option to enable various optimizations including minification and tree shaking.

@philippkuehn
Copy link
Contributor

@dannyrb Okay, so I do not have to do anything? 👀 Currently I'm building umd, commonjs and esm versions of all packages with rollup.

@PhouvanhKCSV PhouvanhKCSV changed the title Do you plan to have tree-shaking for Tiptap? "Tree-Shaking" support for TipTap Sep 7, 2018
@PhouvanhKCSV
Copy link
Contributor Author

PhouvanhKCSV commented Sep 7, 2018

@dannyrb @philippkuehn That is what i mean, sorry for my bad explanation.

webpack bundle analyzer - google chrome_4

As you can see, i did not use any "code (highlighting)" extensions like "CodeBlockNode, CodeMark, CodeBlockHighlightNode" but a big "highlight.js" is still bundled in the vendor file.

@dannyrb
Copy link

dannyrb commented Sep 7, 2018

@philippkuehn, I think the only thing you would need to add (for consumers of your library that are using WebPack) is the sideEffects information in package.json:

{
  "name": "tiptap-packages",
  "sideEffects": false,
   ...
}

https://webpack.js.org/guides/tree-shaking/#mark-the-file-as-side-effect-free

@PhouvanhKCSV, I think you need to import:

import tiptap from 'tiptap/dist/tiptap.esm.js' instead of
import tiptap from 'tiptap'

Someone with more knowledge in this area might be able to help further. I only have a passing knowledge of most of this 😅

@philippkuehn
Copy link
Contributor

@PhouvanhKCSV I've released v0.10.1 and added this sideEffects option to the extenstions package.

Maybe you could test it again?

@PhouvanhKCSV
Copy link
Contributor Author

PhouvanhKCSV commented Sep 7, 2018

@philippkuehn Still the same. I've created a test project, please take a look at this https://github.com/PhouvanhKCSV/vue-tiptap-test

@dannyrb
Copy link

dannyrb commented Sep 7, 2018

I think @philippkuehn has done everything he needs to. This is likely a configuration/setup issue. Here's a Medium article that may help?

https://medium.com/@joanxie/utilize-webpacks-tree-shaking-in-your-vue-application-a0dc63c0dfac

This could be the culprit?

  1. Ensure that your source code files are still in their ES module form when loaded by Babel
    Both es2015 and env presets will by default transpile the ES module syntax to the CommonJS module format. Ensure that both presets have "modules": false set.

I hate tagging him, because I know he has infinitely better things to do, but I'm sure @TheLarkInn or someone else from the WebPack team would be able to offer insight.

@philippkuehn
Copy link
Contributor

Another solution I think about is to move all extension to its own package.

import BlockquoteNode from 'tiptap-bockquote-extension'
import BulletListNode from 'tiptap-bulletlist-extension'
import HardBreakNode from 'tiptap-hardbreak-extension'
import HeadingNode from 'tiptap-heading-extension'
// ...

That would solve this issue too but I'm not sure if this is »too much«!? 🤔

@TheLarkInn
Copy link

TheLarkInn commented Sep 11, 2018 via email

@erickwilder
Copy link
Contributor

I have the same issue here. I've created a test repo with a reproducible example:
https://github.com/erickwilder/tiptap-treeshaking

I'm willing to help you to investigate if this is an issue with tiptap or something that could be fixed by tweaking the project configuration. Bear in mind that the test repo was created with some of the defaults provided by @vue/cli and I assume that their work to optimize the production bundle took a lot in consideration regarding correct bundling.

@erickwilder erickwilder mentioned this issue Oct 10, 2018
5 tasks
@philippkuehn
Copy link
Contributor

@erickwilder Thank you for this repo! @vue/cli should definitely support tree shaking so maybe there is still something to do on my side.

@erickwilder
Copy link
Contributor

erickwilder commented Oct 10, 2018

@philippkuehn I think that the issue lies down in the fact that tiptap-extensions is using the full lowlight library even though the authors recommend to use just what you really need to:

I do not suggest using the pre-built files or requiring lowlight in the browser as that would include a 684kb (196kb GZipped) file.

Instead, require lowlight/lib/core, and include only the used highlighters. For example:

var low = require('lowlight/lib/core');
var js = require('highlight.js/lib/languages/javascript');

low.registerLanguage('javascript', js);

low.highlight('js', '"use strict";');

I didn't have the time yet but I'll try to patch the CodeBlockHighlight.js file to come up with a way to lazy-load required files.

@philippkuehn
Copy link
Contributor

Yeah. First I thought that it would be nice to pass some names of languages to lazy load the required modules within the extension. But I think lazy load won‘t work here because of the build setup with rollup. We would like to get this magic dynamic imports from webpack here to move these to its own chunks.

Instead I think of loading the required languages on user side and pass these to the extension.

@erickwilder
Copy link
Contributor

I agree. Before I start diving into code, let's just agree with the design of the interface of it. One possible idea could be something like passing a list previously registered languages when calling CodeBlockHIghlight's constructor:

extensions: [
  new CodeBlockHighLight({languages: [javascript, markdown, python]}
]

@philippkuehn
Copy link
Contributor

Yes, that syntax would be perfect. But unfortunately this will not be possible with lazy loading. But maybe I'm wrong.

So I thought of moving the import stuff to the user side? 🤷‍♂️

extensions: [
  new CodeBlockHighLight({
    languages: {
      javascript: require('highlight.js/lib/languages/javascript'),
      markdown: require('highlight.js/lib/languages/markdown'),
      python: require('highlight.js/lib/languages/python'),
    },
  }),
]

@erickwilder
Copy link
Contributor

erickwilder commented Oct 11, 2018 via email

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 a pull request may close this issue.

5 participants