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

Build tools improvement: api-extractor for bundling dts #125

Merged
merged 5 commits into from
Apr 30, 2023

Conversation

fnlctrl
Copy link
Contributor

@fnlctrl fnlctrl commented Apr 16, 2023

Why

Typings

Current dist is inconsitent for typescript usage, e.g. this example from quill repo:

import {
  Blot,
  BlotConstructor,
} from 'parchment/dist/typings/blot/abstract/blot';

could have simply been

import type { Blot, BlotConstructor } from 'parchment';

Module format

Currently dist/parchment.js is umd format. It would have been nice to have esm format too.

Whats changed

  • Vite is used for bundling js that support both umd and esm.

  • api-extractor is used for bundling .d.ts. dist/typings is removed. api-extractor.json is added for config.

  • Missing type exports like Blot and BlotConstructor found by api-extractor are added in the index file (src/parchment.ts)

  • tsconfig.json is modified.

    • "emitDeclarationOnly": true is added since tsc is not used for runtime codegen (vite/esbuild are used instead).
    • "moduleResolution": "node" is added for vite config in ts.
    • "exclude" removed and "include" added because it's simpler.
  • Added overrides "semver-regex": "^3.1.4" and "http-cache-semantics": "^4.1.1". This fixes some npm audit issues.

Before

  • dist/
    • typings/
    • parchment.js
    • parchment.js.map

After

  • dist/
    • parchment.d.ts
    • parchment.mjs
    • parchment.mjs.map
    • parchment.umd.js
    • parchment.umd.js.map

@fnlctrl
Copy link
Contributor Author

fnlctrl commented Apr 17, 2023

@luin Could you help take a look?
In our current project we’re trying to import quill from TS source since it hasn’t had new versions for a long time. Everything works fine except for .d.ts bundling - we need it because we’re building on top of quill for an internal package.
Let me know if there anything that you don’t like about this PR and I’ll update accordingly. You don't need to publish a new version for this if it's too much hassle - we can install from github directly. We just need this to be merged to not install from our own fork.
I can make a similar PR for quill-delta to update js/.d.ts bundling as well.

@luin
Copy link
Member

luin commented Apr 18, 2023

Hey @fnlctrl 👋

Thank you for the PR! I'm not sure if it's a good idea to provide both UMD/CJS and ESM, as I was under the impression that bundlers have issues dealing with them (ex webpack/webpack#5756). Additionally, this introduces the dual package hazard where we rely on instanceof heavily when dealing with Blot typings.

We should maintain a consistent tech stack across all Quill repositories. Therefore, I'd suggest avoiding the introduction of Vite at this time solely for ESM support. In the future, we could consider transitioning the package to ESM only.

@fnlctrl
Copy link
Contributor Author

fnlctrl commented Apr 18, 2023

@luin Thank you for taking the time to reivew! Here are my thoughts:

For dual umd/esm builds:

As for the webpack issue, I think it only affects default export:
The equivalent of import Default from 'module''s should be require('module').default. It shouldn't affect named exports.
Since Parchment only contains named exports, this wouldn't be an issue.

As for the dual package hazard, if I'm not mistaken, Parchment is currently only meant to be run in the browser. So it won't be running directly in node and thus subject to node's issue. For people using umd via <script> tag it's irrelevant. For people using modern bundlers (that should load esm by default), it's up to bundler's behavior to deal with simultaneous use of require() and import on the same module. I've made a test on the latest webpack and it doesn't exhibit the dual package hazard: https://stackblitz.com/edit/github-lqh929?file=src%2Findex.js

image

This is just my personal opinion, but using require() along with import is bad practice. Still, we can put a caveat in the readme for people that really want to do this.

I think the tree-shaking benefit of dual umd/esm outweights the above (self-inflicted) require issue. Retaining umd provides more flexibility and is backwards compatible, i.e. releasing this change shouldn't require major version bump.

For vite

If in the future we want ESM only build, webpack couldn't do the job since it doesn't support outputting ESM (webpack/webpack#2933). And considering webpack being retired in favor of turbopack (Tobias Koppers working on turbopack now), I don't think it's going to support it in the forseeable future, so a switch is inevitable (turbpack/vite/etc.).

If you consider vite to be overkill, I can switch to plain esbuild that can also output dual UMD/ESM.

If you still don't like the idea of dual UMD/ESM, I can remove the vite-related stuff and only keep api-extractor.

Thank you again!

@luin
Copy link
Member

luin commented Apr 19, 2023

As for the webpack issue, I think it only affects default export

quilljs/delta has a default export and I wanted to keep the tech stack identical between them. However, upon further consideration, quilljs/delta can remain as a CommonJS package to avoid the dual package hazard issue and likely the Webpack issue as well since it needs to run with Node.js and has default exports. quilljs/parchment can be built as both UMD and ESM, as outlined in this PR.

As for the dual package hazard, if I'm not mistaken, Parchment is currently only meant to be run in the browser.

You are right about this.

If in the future we want ESM only build, webpack couldn't do the job since it doesn't support outputting ESM (webpack/webpack#2933).

Thanks for the reference. I can now see it makes sense to introduce Vite but I'll do more tests later this week or next week to make sure exporting ESM won't introduce breaking changes. Please let me know if you happen to have some insights about this. Thanks!

@fnlctrl
Copy link
Contributor Author

fnlctrl commented Apr 23, 2023

Thank you! I didn't know much about dual package hazard before and was just taking the multi-format output ("formats" option) offered by rollup/vite for granted. This really is tricky, but I think we can make the problem simpler by not having default exports.

Since quill-delta is exporting more than the Delta class, it could make sense to remove the default export (in a major version bump), so that Op, OpIterator, AttributeMap don't have to be bound to the Delta class and are exported separately instead. We can also move the normalizeDelta function currently in quill repo (https://github.com/quilljs/quill/blob/6fb1532fbdcfb2d5df4830a81e707160a72da47b/core/editor.ts#L375-L383) to the delta package as a named export. Then we would have consistent behavior for both require and import:

import { Delta, Op, normalizeDelta } from "quill-delta";
const { Delta, Op, normalizeDelta } = require("quill-delta");

As for the dual package hazard (running in node), since everything can be considered JSON data (Delta, Op, AttributeMap) and they typically don't need instanceof comparisons (e.g. in the quill repo, no instanceof used for delta), the only down side would be possible extra code execution between the CommonJS and ES module versions of a package - which shouldn't be a big issue as quill-delta is a very lightweight package.

Copy link
Member

@luin luin left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! I left two comments. Let's focus this PR on exporting the missing types.

For quill-delta, I'd keep it as CJS as I don't see enough benefits to publish it as a dural package.

package.json Outdated Show resolved Hide resolved
vite.config.ts Outdated
@@ -0,0 +1,13 @@
import { defineConfig } from 'vite';

export default defineConfig({
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 migrating to Vite is the right direction but I'd make a separate PR for it and also should remove webpack.

Copy link
Contributor Author

@fnlctrl fnlctrl Apr 26, 2023

Choose a reason for hiding this comment

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

Removed, will make a separate PR.
I tried removing webpack but found that karma config is still dependent on it. Maybe I should change the test script so that build is run before testing?

package.json Outdated Show resolved Hide resolved
src/parchment.ts Outdated Show resolved Hide resolved
@fnlctrl fnlctrl changed the title Build tools improvement: api-extractor for bundling dts, and vite for bundling js Build tools improvement: api-extractor for bundling dts Apr 26, 2023
@fnlctrl
Copy link
Contributor Author

fnlctrl commented Apr 27, 2023

@luin The tests seems to have stuck for 5 hours... Probably should configure a timeout or something - otherwise it would probably burn through the github actions quota.

@luin luin merged commit 92c04e3 into slab:main Apr 30, 2023
1 check failed
@luin
Copy link
Member

luin commented Apr 30, 2023

Created a PR for the timeout issue: #126

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

2 participants