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

sewing-kit-next build #4424

Merged
merged 7 commits into from
Aug 26, 2021
Merged

sewing-kit-next build #4424

merged 7 commits into from
Aug 26, 2021

Conversation

BPScott
Copy link
Member

@BPScott BPScott commented Aug 24, 2021

WHY are these changes introduced?

Use sewing-kit-next for our build process, rather than having a bespoke rollup config

Being a showcase for the modern (albeit getting the breaking changes in as part of a major version bump.
Migrating from a bespoke rollup build to use sewing-kit-next to perform the build.

Parts of this are still a bit lumpy but will be able to be improved without any breaking change in the near future.

WHAT is this pull request doing?

This PR replaces our bespoke rollup build with using sewing-kit-next (which still uses rollup under the hood). This doesn't look too different at the moment - we still need bespoke code for css compilation for instance and rollup plugins for image handliing, but in the near future this code shall be extracted into a reusable skn plugin that configures rollup in an opinionated way.

Previously package content was built into dist, and temp, cache and storybook output was placed into build. The sewing-kit-next plugin we're using is opinionated and places package content into the build folder. Thus we have to find somewhere else for those temp/cache/storybook files to live. The following changes have been made:

  • The temp, cache and storybook files folder moves from /build to /build-internal
  • The package content folder moves from /dist to /build

This change of the package content location means that consumers who previously imported styles.css now need to use the new path

- @import '@shopify/polaris/dist/styles.css';
+ @import '@shopify/polaris/build/esm/styles.css';

@ghost ghost added the cla-needed Added by a bot. Contributor needs to sign the CLA Agreement. label Aug 24, 2021
@@ -2,8 +2,12 @@
node_modules
/.sewing-kit
/build
/dist
/build-internal
Copy link
Member

Choose a reason for hiding this comment

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

build-internal is a much better place. Thanks @BPScott

Comment on lines +7 to +8
// Run Sk build to generate everything
run(`yarn run skn build`);
Copy link
Member

@alex-page alex-page Aug 24, 2021

Choose a reason for hiding this comment

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

Love this clean up with the preAndPostBuildPlugin. Could we remove this file all together and just run this in the package.json?

@ghost ghost removed the cla-needed Added by a bot. Contributor needs to sign the CLA Agreement. label Aug 24, 2021
@BPScott
Copy link
Member Author

BPScott commented Aug 24, 2021

Running into some surprises with the style.css load order being changed around, I'll be digging into that, but the rest is seems to be good

Copy link
Member

@kyledurand kyledurand left a comment

Choose a reason for hiding this comment

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

Love this Ben! Thanks for walking me through it all ❤️

- Temp file folder moves from /build to /build-internal
- Build output to be published folder moves from /dist to /build
- css file location changed from /dist/styles.css to
  build/esm/styles.css
- Remove the concept of having the build-only tsconfig.build.json.
  tsconfig.json is build as part of the build, and puts test types files
  into the buiild folder. We exclude those files with the files array in
  package.json
When using preserveModules, the array of bundles passed to
generateBundle contains every file, instead of just the entrypoint. this
means we need a different tactic to build the ordered list of imported
css content. Instead of looking at the modules list of the single output
file, we have find the entrypoint file, and find its imports, then
lookup the files that imports and so on to build the ordered list of
imports.
@BPScott
Copy link
Member Author

BPScott commented Aug 26, 2021

Found the root cause of the styles.css order mangling issue and found the fix (by which I mean I remembered others ran into a similar issue back in the day in egoist/rollup-plugin-postcss#295). See e8f5edb

@dahukish This is super important: Can you please apply that fix within global-nav and within the styles plugin that you're extracting into sewing-kit-next. Without this fix the ordering of generated styles.css files is straight up totally wrong.

@BPScott
Copy link
Member Author

BPScott commented Aug 26, 2021

Merging this with failing checks as they're expected.

  • changelog is expected to fail as I'm put changelog entries into UNRELEASED-v7.md instead of UNRELEASED.md
  • size-check is expected to fail as I've changed the build output location compared to master (dist -> build). This is going to continue to be fail till we merge this branch into main.

@BPScott BPScott merged commit 4076f86 into v7.0.0-release Aug 26, 2021
@BPScott BPScott deleted the skn-build branch August 26, 2021 02:44
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.

3 participants