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

Update build logic to a real build system #17

Merged
merged 16 commits into from
Aug 26, 2019

Conversation

JasonVMo
Copy link
Contributor

This changes the build logic to use just + beachball. The initial source was copied from fabric but the change has some additional twists:

  • Changed jest tests to use babel vs. ts-jest. The only thing lost here is type checking which is already covered by the typescript build.
  • Added webpack.config.js files as appropriate to get things building. Webpack config is using the just framework rather than the bespoke approach of fabric
  • Moved demo from packages to apps. This follows the approach of fabric.
  • Added webpack.serve.config.js to get that running correctly

This also includes the incremental build support Ken added for Fabric with his lerna change. Now builds will clean, lint, build, test, and run webpack.

It is likely there are still places where the build logic can be simplified to be closer to standard approaches. Wherever we can cut things out we should unless there is a good reason for them.

Issues fixed in this change:

I also opened Issue #16 to cover switching from tslint to eslint

Copy link
Member

@kenotron kenotron left a comment

Choose a reason for hiding this comment

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

Some comments left about the extraneous change file conversion scripts. In general, looks okay... since they're from UI Fabric web :)

import { mergeSettings } from '@uifabric/theme-settings';
import { useAsPressable } from '../Pressable';
import { useAsPressable } from '../Pressable/index';
Copy link
Member

Choose a reason for hiding this comment

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

BTW, are these changes really needed? We shouldn't need to specify index like this. If we find that the compilation fails because of this, we should fix the configs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At some point I was hitting errors saying that these weren't targeting a physical file. I think in Fabric there is a forwarding file for controls. So there is a Pressable.ts that imports Pressable/index (I think).

scripts/convert-change-files.js Outdated Show resolved Hide resolved
scripts/create-component.js Outdated Show resolved Hide resolved
scripts/lint-staged/auto-convert-change-files.js Outdated Show resolved Hide resolved
@JasonVMo JasonVMo merged commit 13f23a6 into microsoft:master Aug 26, 2019
This was referenced Aug 26, 2019
@JasonVMo JasonVMo deleted the build-logic branch August 26, 2019 22:13
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

3 participants