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: bundle with esbuild #257

Merged
merged 17 commits into from
Jan 24, 2022
Merged

build: bundle with esbuild #257

merged 17 commits into from
Jan 24, 2022

Conversation

Shinigami92
Copy link
Member

No description provided.

@Shinigami92 Shinigami92 added the c: chore PR that doesn't affect the runtime behavior label Jan 22, 2022
@Shinigami92 Shinigami92 self-assigned this Jan 22, 2022
@Shinigami92 Shinigami92 marked this pull request as ready for review January 22, 2022 12:09
@Shinigami92 Shinigami92 requested a review from a team as a code owner January 22, 2022 12:09
@Shinigami92
Copy link
Member Author

@knightly-bot build this

@damienwebdev
Copy link
Member

damienwebdev commented Jan 22, 2022

As I am unfamiliar with the various types of packaging mechanics in NodeJS, I found it helpful to ref this packaging document when reviewing this

@damienwebdev
Copy link
Member

This is missing a README update.

package.json Outdated Show resolved Hide resolved
@damienwebdev
Copy link
Member

damienwebdev commented Jan 22, 2022

This also breaks browser builds (or at least any existing documentation for them). What should the migration path be? Is it possible to output to dist backward compatibly?

@damienwebdev
Copy link
Member

damienwebdev commented Jan 22, 2022

This also breaks the docs.

damien@DESKTOP-N7ABP4H:~/graycore/faker.js$ npm run build && npm run docs:dev

> @faker-js/faker@6.0.0-alpha.3 build
> run-s build:clean build:code build:types


> @faker-js/faker@6.0.0-alpha.3 build:clean
> rimraf lib


> @faker-js/faker@6.0.0-alpha.3 build:code
> esno ./scripts/bundle.ts

Building library for node (cjs)...
Building library for node type=module (esm)...

> @faker-js/faker@6.0.0-alpha.3 build:types
> tsc --emitDeclarationOnly --outDir lib/types


> @faker-js/faker@6.0.0-alpha.3 docs:dev
> vitepress dev docs

vitepress v0.21.6

  > Local: http://localhost:3000/
  > Network: use `--host` to expose
10:31:37 AM [vite] Internal server error: Failed to resolve import "../../../../dist/faker" from "docs/.vitepress/theme/components/Playground.vue". Does the file exist?
  Plugin: vite:import-analysis
  File: /home/damien/graycore/faker.js/docs/.vitepress/theme/components/Playground.vue
  9  |  const ready = ref(false);
  10 |  
  11 |  import('../../../../dist/faker').then((_faker) => {
     |         ^
  12 |    window.faker = _faker.default;
  13 |    faker.value = _faker.default;
      at formatError (/home/damien/graycore/faker.js/node_modules/.pnpm/vite@2.7.13/node_modules/vite/dist/node/chunks/dep-f5552faa.js:36769:46)
      at TransformContext.error (/home/damien/graycore/faker.js/node_modules/.pnpm/vite@2.7.13/node_modules/vite/dist/node/chunks/dep-f5552faa.js:36765:19)
      at normalizeUrl (/home/damien/graycore/faker.js/node_modules/.pnpm/vite@2.7.13/node_modules/vite/dist/node/chunks/dep-f5552faa.js:73703:26)
      at async TransformContext.transform (/home/damien/graycore/faker.js/node_modules/.pnpm/vite@2.7.13/node_modules/vite/dist/node/chunks/dep-f5552faa.js:73843:57)
      at async Object.transform (/home/damien/graycore/faker.js/node_modules/.pnpm/vite@2.7.13/node_modules/vite/dist/node/chunks/dep-f5552faa.js:36985:30)
      at async doTransform (/home/damien/graycore/faker.js/node_modules/.pnpm/vite@2.7.13/node_modules/vite/dist/node/chunks/dep-f5552faa.js:52060:29)
<script setup>
import { ref } from 'vue';

const faker = ref();
const ready = ref(false);

import('../../../../lib/esm/index').then((_faker) => {
  window.faker = _faker.default;
  faker.value = _faker.default;
  ready.value = true;
});
</script>

Seems to fix the issue in docs/.vitepress/theme/components/Playground.vue but I would need @JessicaSachs to verify.

@damienwebdev
Copy link
Member

damienwebdev commented Jan 22, 2022

@Shinigami92 I want to ensure we're clear on whether or not this drops support for es5 in browser builds, do you know if it does?

@Shinigami92
Copy link
Member Author

@damienwebdev

As I am unfamiliar with the various types of packaging mechanics in NodeJS, I found it helpful to ref this packaging document when reviewing this

Oh yeah, I also did know nothing about this before this PR.

This is missing a README update.

Oh yeah, you are right. I will work on that today or tomorrow 👍

This also breaks browser builds (or at least any existing documentation for them). What should the migration path be? Is it possible to output to dist backward compatibly?

Potentially yes, it could break the browser support. That's why I would like to have a 6.0.0-alpha.4 release to test out if we can do something with https://unpkg.com
If this doesn't work out of the box, we may need to add a third bundle for iife (See https://esbuild.github.io/api/#format-iife)
But this would mean additional around 10 MiB, because the locales needs always to be duplicated for each bundle.

I'm thinking about to drop browser support for now and bring it back in later when we found a much better way of async loading in locales. (This may not be that much difficult.)

This also breaks the docs.

Oh 😲, thanks I need to have a look into that 👍

I want to ensure we're clear on whether or not this drops support for es5 in browser builds, do you know if it does?

I need also to find out. But if we really provide an iife bundle, we can just target es5. So really easy with that 👍.

@Shinigami92 Shinigami92 marked this pull request as draft January 22, 2022 18:43
@Shinigami92
Copy link
Member Author

@damienwebdev Currently we provide sourcemaps via esbuild
Do we need to provide these? Because these are really big and blow up the bundle size.

@Shinigami92 Shinigami92 marked this pull request as ready for review January 22, 2022 19:26
@Shinigami92 Shinigami92 requested review from damienwebdev, JessicaSachs and a team January 22, 2022 19:26
@damienwebdev
Copy link
Member

@Shinigami92 can you doc here the before and after size of the the bundles so that we can compare?

@Shinigami92
Copy link
Member Author

@damienwebdev

I checked three things:

  1. esbuild with sourcemaps
    package size: 3.4 MB | unpacked size: 16.6 MB | total files: 5835
    esbuild-with-sourcemap.log

  2. esbuild without sourcemaps
    package size: 1.6 MB | unpacked size: 7.0 MB | total files: 3871
    esbuild-without-sourcemap.log

  3. main with tsc + gulp browser no sourcemap (current state)
    package size: 3.1 MB | unpacked size: 14.5 MB | total files: 3803
    tsc-and-gulp-browser-without-sourcemap.log


Please note that the esbuild process includes cjs and esm but no browser and the main includes cjs and browser but no esm
Also note that the current main doesn't provide source maps. So we could really leave them out for now and then we would reduce the bundle size by half!

And in addition to that, I'm already thinking about to split manually the locales into it's own folder and fetch them on demand.
But I assume we need to convert them then into json files or yaml if possible to make them even smaller.

But this would be definitely something for another PR

@Shinigami92 Shinigami92 linked an issue Jan 23, 2022 that may be closed by this pull request
@Shinigami92
Copy link
Member Author

Shinigami92 commented Jan 24, 2022

I created a test npm package: https://www.npmjs.com/package/@shinigami92/faker/v/6.0.0-alpha.4-test-esbuild-1

It can be tested with https://unpkg.com/@shinigami92/faker@6.0.0-alpha.4-test-esbuild-1/lib/esm/index.js

Just create a simple e.g. vite project and use

<!DOCTYPE html>
<html>
  <body>
    <script type="module">
      import { faker } from "https://unpkg.com/@shinigami92/faker@6.0.0-alpha.4-test-esbuild-1/lib/esm/index.js";

      console.log(faker.name.firstName());
    </script>
  </body>
</html>

with package.json

{
  "name": "faker-browser-test",
  "scripts": {
    "test": "vite"
  },
  "devDependencies": {
    "vite": "~2.7.13"
  }
}

CONTRIBUTING.md Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
test/address.spec.ts Outdated Show resolved Hide resolved
Base automatically changed from migrate-vendor to main January 24, 2022 18:30
@Shinigami92 Shinigami92 marked this pull request as draft January 24, 2022 18:36
Copy link
Member

@damienwebdev damienwebdev left a comment

Choose a reason for hiding this comment

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

lgtm

@Shinigami92 Shinigami92 requested a review from prisis January 24, 2022 19:01
@Shinigami92 Shinigami92 marked this pull request as ready for review January 24, 2022 19:02
@Shinigami92 Shinigami92 merged commit 12e3365 into main Jan 24, 2022
@Shinigami92 Shinigami92 deleted the bundle-with-esbuild branch January 24, 2022 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: chore PR that doesn't affect the runtime behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setup a new bundling process
3 participants