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

feat: angular package format #1474

Merged
merged 30 commits into from
May 22, 2019
Merged

feat: angular package format #1474

merged 30 commits into from
May 22, 2019

Conversation

yggg
Copy link
Contributor

@yggg yggg commented May 21, 2019

Please read and mark the following check list before creating a pull request:

Short description of what this resolves:

Adds esm2015, fesm2015, fesm5 into packages distributives.

BREAKING CHANGE:
Everything not mentioned in packages public_api.ts now private. Direct imports from (@nebular/theme/*) will stop working.

yggg added 17 commits May 14, 2019 20:10
Type annotation on fields with forwardRef lead to 'Cannot access X
before initialization' error in es2015 build.
This change also fixes circular dependency warning.
Prevents "Maximum call stack size exceeded" when building with
"flatModuleOutFile".
angular#25226
Fixes "Cannot access NbLayoutComponent before initialization".
When building with ES2015 target, "forwardRef" in header breaks.
angular#30106.
They will be inserted during the build
Enables more aggressive webpack optimizations
@yggg yggg requested a review from nnixaa May 21, 2019 09:37
@codecov
Copy link

codecov bot commented May 21, 2019

Codecov Report

Merging #1474 into master will increase coverage by 0.34%.
The diff coverage is 95.43%.

@@            Coverage Diff             @@
##           master    #1474      +/-   ##
==========================================
+ Coverage   82.95%   83.29%   +0.34%     
==========================================
  Files         240      230      -10     
  Lines        7332     7489     +157     
  Branches      659      659              
==========================================
+ Hits         6082     6238     +156     
- Misses       1060     1061       +1     
  Partials      190      190
Impacted Files Coverage Δ
...ramework/theme/components/window/window.options.ts 100% <ø> (ø) ⬆️
...nts/cdk/overlay/dynamic/dynamic-overlay-handler.ts 100% <ø> (ø) ⬆️
...rc/framework/theme/components/dialog/dialog-ref.ts 100% <ø> (ø) ⬆️
...framework/auth/strategies/auth-strategy-options.ts 100% <ø> (ø) ⬆️
...rk/theme/components/cdk/overlay/overlay-service.ts 100% <ø> (ø)
...ramework/theme/components/cdk/bidi/bidi-service.ts 100% <ø> (ø)
src/framework/auth/auth.options.ts 100% <ø> (ø) ⬆️
.../theme/components/cdk/platform/platform-service.ts 100% <ø> (ø)
...calendar-day-picker/calendar-day-cell.component.ts 94.11% <100%> (ø) ⬆️
...heme/components/datepicker/datepicker.component.ts 90.1% <100%> (+0.22%) ⬆️
... and 66 more

@@ -17,8 +17,10 @@ packages_smoke() {
cp -r ../nebular/src/.lib/* node_modules/@nebular

echo "Verifying application build"
npm run build --prod --aot
npm run build -- --prod --aot
Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure we don't need --aot flag here because it's already listed as aot: true in angular.json for production environment. Could you please, remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in 3ef9057

DEV_DOCS.md Outdated
@@ -27,6 +27,7 @@
- add the package into bump-version.ts which bumps package version and its dependencies
- add the package external dependencies into rollup-config.ts which gives rollup capability build package correctly
- add the package into bundle.ts which build umd modules for our packages
- add the package into `JS_PACKAGES` in config.ts which used to add es2015 bundles for our packages
Copy link
Member

Choose a reason for hiding this comment

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

Could you please mention where that config.ts lives? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in 9d0cebd

@@ -6,7 +6,7 @@ import * as replace from 'gulp-replace';
import * as minimist from 'minimist';
import { capitalize, dasherize } from '@angular-devkit/core/src/utils/strings';

import { replaceScssWithCss } from './copy-sources';
// import { replaceScssWithCss } from './copy-sources';
Copy link
Member

Choose a reason for hiding this comment

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

I think that line has to be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in c15e9b6

@@ -218,7 +218,7 @@ task('change-prefix', ['copy-build-dir-and-rename', 'generate-ts-config', 'patch

return stream
.pipe(dest(BUILD_DIR))
.on('end', replaceScssWithCss);
// .on('end', replaceScssWithCss);
Copy link
Member

Choose a reason for hiding this comment

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

I think that line has to be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in c15e9b6

@yggg yggg requested a review from Tibing May 22, 2019 08:26
@Tibing
Copy link
Member

Tibing commented May 22, 2019

I personally like the implementation 😄 👍

@Tibing
Copy link
Member

Tibing commented May 22, 2019

LGTM

@yggg
Copy link
Contributor Author

yggg commented May 22, 2019

Thanks 🙂
But hopefully we'll switch to bazel and packagr soon.

@yggg yggg merged commit 76ad28b into akveo:master May 22, 2019
yggg added a commit that referenced this pull request May 27, 2019
yggg added a commit that referenced this pull request Jun 5, 2019
yggg added a commit that referenced this pull request Jun 6, 2019
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.

2 participants