Separate marked into modules #1563
Conversation
|
This pull request is being automatically deployed with ZEIT Now (learn more). 🔍 Inspect: https://zeit.co/markedjs/markedjs/8qhlyysmp |
|
Running the benchmark on I guess we have to decide if it is worth the slow down to have easier development. There may also be some way that we can speed it up but I can't think of a way off the top of my head. |
package.json
Outdated
| @@ -4,6 +4,7 @@ | |||
| "author": "Christopher Jeffrey", | |||
| "version": "0.7.0", | |||
| "main": "./lib/marked.js", | |||
| "module": "./src/marked.js", | |||
There was a problem hiding this comment.
You should probably use "type: module" to make this work with Node.js (however it is still experimental)
https://nodejs.org/api/esm.html#esm_code_package_json_code_code_type_code_field
There was a problem hiding this comment.
@UziTech Why did you mark this as "resolved"? This does not follow the Node.js standard for ESM.
There was a problem hiding this comment.
I just realized this PR is not using ESM at all?
There was a problem hiding this comment.
Ya ES Modules are still experimental and I found that CommonJS is faster in node lts right now.
It wouldn't be hard to change to ESM in the future.
There was a problem hiding this comment.
@UziTech I guess I don't see the difference between this PR and #1349 or #1452
- move to ESM and what not #1349 implements ESM source files and separates into separate modules instead of one source file
- feat: ES Module entry point #1452 implements CJS => ESM as an output via rollup
We rejected both of them with the plan to wait for ESM to go stable in Node. So why make this PR?
There was a problem hiding this comment.
This PR doesn't move to ESM it uses CommonJS for node. Other than that they are pretty much the same.
When I started creating this PR it was more or less just to test the speed of making marked modular and it turns out moving to CommonJS on node made it a bit faster , and using rollup and babel to output browser versions kept the speed about the same.
|
Yeah. @chjj did mention that he did a lot of JS voodoo to get Marked to be as fast is it is. So, just to make sure I'm seeing this correctly. We're splitting things up - like the issues we've put out for updating the architecture. However, we are still compiling back to something that's backward compatible, which is would give us that move before the 1.0 release (where I think we have that forecasted)? The splits seem perfectly rational. Any idea what's causing that shift?? (Like what non-API changes are we making to causing it to run slower??) |
|
I'm trying to move the different components (e.g. <script src="https://cdn.jsdelivr.net/gh/markedjs/marked/lib/marked.js"></script>I'm looking into the slowdown. I'm guessing it has to do with babel using slow polyfills. |
|
It must be babel. When I kept the code as es5 and just used rollup to combine the modules the speed difference was negligible. |
|
I have done a lot of testing and here is my findings:
bench: |
To prevent changes to compiled files in pull requests the user can just run `npm run build:reset`
|
@davisjam I removed the redos test with However, with this move to make marked more modular I did move the regular expressions into their own file so we could run some tests on that file to look for vulnerable regular expressions. |
|
Appreciate the separation of the regex. I also do like the idea being able to take "automatic" advantage of future language improvements. I'm not as embedded in the JS world, but I imagine the move to...pure (??)... native classes would be almost inevitable if only for adoption reasons. Leave that to the more JS steeped though. Is ~6s the baseline for the performance tests? |
|
The benchmark time is very dependent on the machine running it but the important thing is the relative difference between each test. |
Marked version: master
Description
Split
marked.jsinto separate modules and uses rollup and babel to combine them to build./lib/marked.jsAlso uses rollup to build
./lib/marked.esm.jsfor anyone who wants to use<script type="module" ...TODO
marked.defaultsis no longer a global in every module, but setting defaults should be backwards compatible.Contributor
Committer
In most cases, this should be a different person than the contributor.