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

angular-cli 6 (with webpack 4) compatibility #3491

Merged
merged 17 commits into from
Jun 7, 2018

Conversation

igor-dv
Copy link
Member

@igor-dv igor-dv commented Apr 26, 2018

Issue: #3399

What I did

  • Upgraded to v6 of the Angular related deps
  • Adopt migration of the angular-cli.json -> angular.json
  • Update peers to >=6

CC. @Quramy @avol-io @bengry

@@ -4,7 +4,7 @@ import { logger } from '@storybook/node-logger';

function isAngularCliInstalled() {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: @angular/cli module is no longer needed, so I prefer such as isBuildAngularInstalled.

@@ -21,12 +21,19 @@ export function getAngularCliWebpackConfigOptions(dirToSearch, appIndex = 0) {
throw new Error('.angular-cli.json must have apps entry.');
}
const appConfig = cliConfig.apps[appIndex];
const { root, styles, scripts } = appConfig;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should read the build options from angular.json (not .angular-cli.json). And the structures of them are not compatible.

const angularJson = JSON.parse(fs.readFileSync(path.resolve(dirToSearch, 'angular.json'), 'utf8'))
const project = angularJson['projects'][angularJson['defaultProject']];
const cliWebpackConfigOptions = {
  buildOptions: project.architect.build.options,
  root: project.root,
  projectRoot: dirToSearch,
  supportES2015: false,
  tsConfig: {
    options: { },
    fileNames: [],
    errors: [],
  },
  tsConfigPath: path.resolve(dirToSearch, 'src/tsconfig.app.json'),
};

I've created the sample repo to extract webpack config object using @angular-devkit/build-angular
https://github.com/Quramy/how-to-extract-webpack-conf-from-angular-cli

"@angular/platform-browser": "^5.2.10",
"@angular/platform-browser-dynamic": "^5.2.10",
"@ngrx/store": "^5.2.0",
"@angular/common": "^6.0.0-rc.5",
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned above, new angular/cli v6 needs not .angular-cli.json but angular.json. I think it's better to re-generate the example project with ng new angular-cli.

Copy link
Member

Choose a reason for hiding this comment

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

Stable 6 version is out now, please upgrade

Copy link
Member Author

@igor-dv igor-dv May 7, 2018

Choose a reason for hiding this comment

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

Yeah, I am on it

…egration

# Conflicts:
#	examples/angular-cli/package.json
#	yarn.lock
@codecov
Copy link

codecov bot commented May 7, 2018

Codecov Report

Merging #3491 into master will decrease coverage by 0.03%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3491      +/-   ##
==========================================
- Coverage   41.45%   41.42%   -0.04%     
==========================================
  Files         458      459       +1     
  Lines        5075     5077       +2     
  Branches      853      855       +2     
==========================================
- Hits         2104     2103       -1     
  Misses       2464     2464              
- Partials      507      510       +3
Impacted Files Coverage Δ
app/angular/src/server/wrapInitialConfig.js 0% <ø> (ø) ⬆️
addons/jest/src/components/Panel.js 0% <ø> (ø) ⬆️
app/angular/src/server/angular-cli_config.js 0% <0%> (ø) ⬆️
app/angular/src/server/angular-cli_utils.js 0% <0%> (ø)
addons/storysource/src/StoryPanel.js 0% <0%> (ø) ⬆️
addons/events/src/components/Event.js 0% <0%> (ø) ⬆️
addons/storysource/src/loader/inject-decorator.js 100% <0%> (ø) ⬆️
addons/storysource/src/loader/generate-helpers.js 100% <0%> (+9.37%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a46d3e8...9b13bfc. Read the comment docs.

@igor-dv
Copy link
Member Author

igor-dv commented May 7, 2018

@Quramy, I have a strange thing with "assets" prop.

I was needed to override it like this:

buildOptions: {
      ...project.architect.build.options,
      assets: [],
    },

Otherwise, I am getting an error from this code:

image

which is strange because assets were generated by angular-cli and they are:

 "assets": [
              "src/favicon.ico",
              "src/assets"
            ]

@igor-dv
Copy link
Member Author

igor-dv commented May 13, 2018

@klimentru1986, after your angular PRs, I assume you can help here =)

@klimentru1986
Copy link
Contributor

klimentru1986 commented May 13, 2018

@igor-dv , i find example in https://github.com/angular/devkit/blob/master/packages/angular_devkit/build_angular/src/utils/normalize-asset-patterns.ts
, rewrite it, and send request #3578

It`s just a bit ugly, but it is prototype.

I think the best way is to rewrite this part of angular-storybook to typescript and use angular-devkit default methods with observable. ))

@daiky00
Copy link

daiky00 commented May 15, 2018

is this going to get fixed?

@@ -0,0 +1,55 @@
import fs from 'fs';
import { basename, dirname, normalize, relative, resolve } from '@angular-devkit/core';
Copy link
Member Author

Choose a reason for hiding this comment

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

If someone doesn't use angular-cli, will this dep still appear? I assume that it won't, that's why we added the isBuildAngularInstalled func

@ndelangen
Copy link
Member

What's the status of this @igor-dv ?

@ndelangen ndelangen added this to the v4.0.0 milestone May 23, 2018
…egration

# Conflicts:
#	app/angular/src/server/config/webpack.config.js
#	examples/angular-cli/package.json
#	package.json
#	yarn.lock
@igor-dv
Copy link
Member Author

igor-dv commented Jun 2, 2018

Something bad happens with the static build after the merge from master 🤔

Looks related to webpack

@igor-dv
Copy link
Member Author

igor-dv commented Jun 3, 2018

@Hypnosphi, do we have any WIP PR with upgrading webpack to 4.10.2 ? Looks like it solves a static build problem here

…egration

# Conflicts:
#	examples/marko-cli/package.json
#	yarn.lock
@igor-dv igor-dv merged commit b4d4704 into master Jun 7, 2018
@igor-dv igor-dv deleted the new-angular-cli-integration branch June 7, 2018 13:56
}

return assetPatterns.map(assetPattern => {
const assetPath = normalize(assetPattern);
Copy link

@santialbo-actimo santialbo-actimo Jun 11, 2018

Choose a reason for hiding this comment

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

Hi @igor-dv sorry to nudge you like this. I'm getting a an error here.
At this point my asset pattern looks like this:

{ glob: '**/*',
  input: 'apps/viewer/src/assets',
  output: '/assets' }

which throws the following error:

TypeError: path.match is not a function

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, will take a look

@TheDevelolper
Copy link

TheDevelolper commented Sep 18, 2021

I'm not sure what's happening with this issue? Is it being looked at? Is it resolved? What's the fix?

my angular.json looks like this:

image

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

Successfully merging this pull request may close these issues.

8 participants