Skip to content
This repository has been archived by the owner on Oct 1, 2024. It is now read-only.

Multi-build support (cjs, esm, esnext) for all quilt packages #1698

Merged
merged 5 commits into from
Dec 21, 2020

Conversation

ismail-syed
Copy link
Contributor

@ismail-syed ismail-syed commented Dec 9, 2020

Description

This PR migrates quilts build system to SK-next (shopify/sewing-kit-next) to provide multiple build outputs (cjs, esm, esnext). Most of this PR is a revive of our previous exploration. The benefit of migrating our build system over to SK-next is because our tooling leverages esnext supported packages to provide treeshaking. In our previous exploration, we also consumed these changes into an app, where we noted significant bundle size decreases.

Why such a big PR? Over 300 file changes

We currently have 69 packages in quilt. Migrating only some and not all will be a very wonky dev experience. Please call me out on parts I can break out 😄 .

Summary of changes

  • Migrate build setup to SK-next
    • A project root sewing-kit.config.ts that encompasses all the workspace level configs. Workspace refers to the whole monorepo, think of yarn workspaces.
    • A config/sewing-kit/index.ts which is used as an SK config that is consumed by all other package level SK-next configs.
    • A sewing-kit.config.ts file for each package
  • Updates various configs as part of the migration (eslint, gitignore, eslintignore, tsconfigs, jest)

Overall CI times

Cache Before After
Cold 9m 12s 18m 15s
Warm 7m 47s 5m 46s

NOTE: Cold builds have regressed ~2x because the builds are generating over twice as many build files than they were previously.

Why do we need to do this anyways?

esnext builds provide better tree-shaking support for our quilt packages . 🎩 'd on shirnk-ray (https://github.com/Shopify/shrink-ray/pull/1718). As expected we're getting noticeable reductions in the vendor bundle size.

image

🎩

The team did a thorough 🎩 during #1538 so much of this is 👍.
I also 🎩 'd on shirnk-ray (https://github.com/Shopify/shrink-ray/pull/1718)

For reviews a simple yarn run build should suffice.

Type of change

  • Patch: Bug (non-breaking change which fixes an issue)
  • Minor: New feature (non-breaking change which adds functionality)
  • Major: Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

@ghost ghost added the cla-needed label Dec 9, 2020
@ismail-syed ismail-syed marked this pull request as draft December 9, 2020 03:44
@ismail-syed ismail-syed changed the title cl Multi build support Multi build support Dec 9, 2020
@ismail-syed ismail-syed force-pushed the multi-build-support branch 4 times, most recently from 531c140 to 3f9f2cf Compare December 9, 2020 15:28
@atesgoral atesgoral mentioned this pull request Dec 9, 2020
1 task
@ismail-syed ismail-syed force-pushed the multi-build-support branch 5 times, most recently from 481a785 to 8d9eb8f Compare December 10, 2020 22:22
@ismail-syed ismail-syed changed the title Multi build support Multi-build support (cjs, esm, esnext) for all quilt packages Dec 10, 2020
@ismail-syed ismail-syed force-pushed the multi-build-support branch 15 times, most recently from a823d3e to 45088f4 Compare December 14, 2020 23:08
@ghost ghost removed the cla-needed label Dec 14, 2020
@ismail-syed
Copy link
Contributor Author

  1. tophat is broken

This seems legit, still looking into this. I recall we had tophat related issues with sk-next built libraries. @gmcgibbon took at stab a fixing this with https://github.com/Shopify/sewing-kit-next/pull/98. I'll see if we can find a solution for quilt.

  1. Creating new package cause unnecessary update to tsconfig.json

Fixed with fef0de7

  1. Run into an error during linting

I wasn't able to reproduce this. Can you see if this works? git clean -fxd && yarn install && yarn run lint

@ismail-syed
Copy link
Contributor Author

ismail-syed commented Dec 18, 2020

🎩 'd on shirnk-ray (https://github.com/Shopify/shrink-ray/pull/1718). As expected we're getting noticeable reductions in the vendor bundle size.

image

Update: Merged/Deployed to staging. All is good 👍

@ismail-syed
Copy link
Contributor Author

@michenly yarn tophat is doesn't integrate well with sk-next's build command yet. It's quite tailored for a tsc build. The way I see it, we have 2 options:

  1. Merge this PR and then address sk-next/yarn tophat.
  2. Address sk-next/yarn tophat integrations first then merge this PR.

Copy link
Contributor

@michenly michenly left a comment

Choose a reason for hiding this comment

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

Tested 2 & 3 again and those seems to be resolved. tophat regression while annoying we can deal with that in a separate PR.

Copy link
Contributor

@marutypes marutypes left a comment

Choose a reason for hiding this comment

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

Code looks good,

Bundle size changes look great, a bit worried about cold-cache build times but seems inescapable given multi-format builds...

@atesgoral atesgoral marked this pull request as ready for review December 19, 2020 23:02
@ismail-syed ismail-syed merged commit 561d741 into master Dec 21, 2020
@ismail-syed ismail-syed deleted the multi-build-support branch December 21, 2020 16:13
@ismail-syed ismail-syed temporarily deployed to production December 21, 2020 16:40 Inactive
@ryanwilsonperkin
Copy link
Member

Hey! 👋🏻 I believe this PR broke @shopify/react-form. I have an issue written up here: #1715

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants