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

📦 Chore(build): migrate to esbuild and change export assignment to esm export #102

Merged
merged 7 commits into from
Oct 26, 2021

Conversation

upupming
Copy link
Member

@upupming upupming commented Oct 4, 2021

Fixes #101, #42

In this PR, I use esbuild to build both cjs and esm bundle for picgo-core. All dependencies are externalized, so the bundle is minimized to only ~280KB:

❯ ls -lh dist/*.js
-rw-r--r--  1 upupming  staff   278K Oct  4 23:11 dist/index.cjs.js
-rw-r--r--  1 upupming  staff   273K Oct  4 23:11 dist/index.esm.js

I have also tried to use rollup, but I found it is too slow to build, and have to use tsc to generate .d.ts files too, because this usage will cause rollup typescript plugin trying to generate dist/src/index.d.ts instead of dist/index.d.ts (see ezolenko/rollup-plugin-typescript2#275 (comment)):

const { version } = require('../../package.json')

So finally I write a custom esbuild script for this task.

@Molunerfinn
Copy link
Member

It's a breaking change since the developer should change the usage of picgo.

We should export commonJS & ESM both

@Molunerfinn
Copy link
Member

the node developer will use const picgo = require('picgo') instead of const picgo = require('picgo').default

@upupming
Copy link
Member Author

upupming commented Oct 8, 2021

@Molunerfinn Since we have changed the export method of PicGo, now user has to use this import method:

// CJS
const { PicGo } = require('picgo') // or const PicGo = require('picgo').PicGo
// ESM
import { PicGo } from 'picgo'

There are a lot of libraries are using this export method, such as liquidjs:

https://github.com/harttle/liquidjs/blob/a525f45b4664ef27d9f835873018f69153891cfc/src/liquid.ts#L20

I think there is a way to make both const PicGo = require('picgo') and const { PicGo } = require('picgo') works. I will try it and report here later
I found that we cannot be compatible with the current import method. If we want to support const picgo = require('picgo'), then it is awkward that we can only export one object, so export any types and internal classes such as Lifecycle is impossible.

Ok, I found a way to hack and support both method. I will give a repo in several minutes.

@upupming
Copy link
Member Author

upupming commented Oct 8, 2021

@Molunerfinn I have done some test that there is a way to support both style import/require, you can see my demo here: https://github.com/upupming/ts-export-test. What do you think, if you agree the method linked in the repo, I will implement it in picgo-core~

@Molunerfinn
Copy link
Member

rollup+umd would be better

@upupming
Copy link
Member Author

upupming commented Oct 8, 2021

@Molunerfinn Do you mean support browser environment in the future? ESBuild can supports UMD, too, but we have to use custom banner and footer, see: evanw/esbuild#819 (comment). I have tried Rollup, but I stuck at getting the correct generated .d.ts structure. All I get is ./dist/src/xxx.d.ts, there is an extra src folder because we tried to import package.json in our code.

I think the switch to esbuild or Rollup is quite easy and will not affect any user. We can build UMD bundle once we don't relay on Node.js environment anymore (such as dynamic require, fs.readFileSync etc.), but at the present, UMD bundle is just too far away from what we can consider.

@Molunerfinn
Copy link
Member

the big problem is that esbuild just supports to es6

@upupming
Copy link
Member Author

upupming commented Oct 8, 2021

@Molunerfinn Yes, but I think we can simply assume that all user has a Node.js environment that supports ES6 at the present.

Anyway, If you prefer rollup, I will change to rollup, the major benefits of esbuild is faster. What do you think?

@Molunerfinn
Copy link
Member

Molunerfinn commented Oct 9, 2021

@Molunerfinn Do you mean support browser environment in the future? ESBuild can supports UMD, too, but we have to use custom banner and footer, see: evanw/esbuild#819 (comment). I have tried Rollup, but I stuck at getting the correct generated .d.ts structure. All I get is ./dist/src/xxx.d.ts, there is an extra src folder because we tried to import package.json in our code.

I think the switch to esbuild or Rollup is quite easy and will not affect any user. We can build UMD bundle once we don't relay on Node.js environment anymore (such as dynamic require, fs.readFileSync etc.), but at the present, UMD bundle is just too far away from what we can consider.

import package.json just wants to use the version. If we use bundler(rollup\esbuild\webpack), we can get the version in build time, and we don't need to import package.json anymore

@upupming
Copy link
Member Author

upupming commented Oct 9, 2021

@Molunerfinn Ok, I know it, and I will change the esbuild to rollup and solve the version problem.

@upupming
Copy link
Member Author

upupming commented Oct 9, 2021

@Molunerfinn I have changed the build tool from esbuild to rollup, here are the build results:

image

@Molunerfinn
Copy link
Member

Nice. Have you tested the clipboard image uploading process in mac or win?

This PR will be merged If I have time to test. And It will be a breaking change since we change the export way. It will be released in v1.5+

@upupming
Copy link
Member Author

upupming commented Oct 9, 2021

@Molunerfinn I have test CLI & vs-picgo integration & PicGo-Electron integration on mac. And also I tested the CLI on Windows, below is the generated ps1 file at the runtime:

image

The clipboard feature works like a charm!

@Molunerfinn Molunerfinn merged commit 2a6cd18 into dev Oct 26, 2021
@Molunerfinn Molunerfinn deleted the feat-esbuild branch October 26, 2021 13:27
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.

Feature request: change export assignment to esm export to allow export types for importing
2 participants