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

CLI-less webpack build #1105

Closed
wants to merge 17 commits into from
Closed

CLI-less webpack build #1105

wants to merge 17 commits into from

Conversation

Sayan751
Copy link
Member

@Sayan751 Sayan751 commented May 23, 2019

This PR resolves #1098.

Summary of changes:

  • Parameters consolidations and other minor changes in webpack.config.js. Removed the limiting options from ts-loader as it prevents compiling *.spec.ts files during test with karma(+webpack).
  • Replaced the au build and run scripts with npm scripts. Affected areas: package.json, aurelia_project/tasks, and release check tests. For pre-build cleanup activity, there is a small gulpfile introduced.
  • Changed tsconfig.json to have typeroots applied for all cases, without that the release checks were failing for couple of suites. Other changes here enable loading a json file with ES6 import syntax.
  • Lastly converted the environment.tss to json files, which are loaded using the app-settings-loader.

Sayan751 added 13 commits May 16, 2019 00:01
Tried a different approach to tackle the enviroment configurations. Instead of a typescript file, which is replaced based on the environment, this strategy uses json file to hold the configuration. The loader takes the basic config file and applies that environmentspecific changes (change environment.json and environment.production.json).
These are not needed anymore, as webpack and webpack-dev-server CLIs are directly used to build and run the app.
- Moved the environment jsons to another directory
- Added options to load json modules in tsconfig
- Changed the imports for environment
- Lastly cleaned up loader with external package
Removed limiting options, so that ts-loader can compilespec.ts files
during tests.
Copy link
Member

@3cp 3cp left a comment

Choose a reason for hiding this comment

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

One more question, I didn't see you remove aurelia-cli from final package.json of webpack app. Is unit tests still using au test command?

skeleton/common/tsconfig.json__if_typescript Outdated Show resolved Hide resolved
},
// @endif
// @if feat.typescript
{ test: /\.ts$/, loader: "ts-loader", options: { reportFiles: [ srcDir+'/**/*.ts'] }, include: srcDir },
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 you accidentally removed this line from master.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, saw your commit message for this one too. I guess the side effect is that you have to turn on typeRoots unconditionally, which is not idea.

Copy link
Member Author

Choose a reason for hiding this comment

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

My earlier take on the typeRoots involving monorepo might be little bit premature, as I have not recently tried to create a monorepo for a aurelia project. I tried that that a year back, and there were massive problems with resolving/loading symlinked packages; the webpack config was crowded with aliases. If this is a blocker, I will try to create a lerna prototype, and report the the findings.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to create a monorepo using au-cli and lerna. I couldn't make it work. It is not really because of the type definition, which makes the problem I suspected earlier. But even after fixing the typeRoots, Aurelia has problems loading the custom elements coming from different plugins. I opened an issue before on similar problem. I think the problems related to loading the symlinked packages needs to corrected before starting with bootstrapping lerna monorepo projects with lerna.

On the other hand, this typing issue with Jasmine is already opened here: DefinitelyTyped/DefinitelyTyped#14569. If type definition of jasmine is got resolved, probably the whole typeRoots thing can be done away with.

Copy link
Member

Choose a reason for hiding this comment

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

I also discovered another bug with limiting ts-loader include. I am running release-check on v1 candidate, where I need to modify it to include: karma ? [srcDir, testDir] : srcDir to fix webpack+karma setup.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I faced that too. Otherwise the *.spec.ts files are never processed, and webpack throws error. I think not having the include for ts-loader is far easier solution.

Copy link
Member

Choose a reason for hiding this comment

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

I see. I will retain the "include" for v1 release. Then we can work on your big refactoring after v1.

@@ -32,10 +32,8 @@
"types": ["node", "jest"],
// @endif

// @if feat.webpack && feat['postcss-typical'] && feat.protractor
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 you accidentally removed some lines?

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I saw your commit message. But we did this conditional typeRoots to do minimum impact for users of lerna (where "./node_modules/@types" does not work).

@3cp
Copy link
Member

3cp commented May 23, 2019

Sidenote: this is technically a breaking change for webpack app. But it's practically not a breaking change, because it doesn't affect/break existing webpack apps. This only affects newly created webpack app.

@Sayan751
Copy link
Member Author

One more question, I didn't see you remove aurelia-cli from final package.json of webpack app. Is unit tests still using au test command?

Yup, those are still based on au test. But it can also be encapsulated in a npm script. As I was focusing on webpack, I actually paid no attention to that.

@3cp
Copy link
Member

3cp commented May 23, 2019

I thought our target is to remove runtime dependency on aurelia-cli for webpack app.

@Sayan751
Copy link
Member Author

I thought our target is to remove runtime dependency on aurelia-cli for webpack app.

I misunderstood it to be a task to remove the au run, and au build for webpack projects. It makes more sense to remove all au flavored commands from webpack apps. I would suggest to have that in a different PR, as this is already quite big. A new PR might also be easy to review. We can still use the same issue to track that.

@3cp
Copy link
Member

3cp commented May 23, 2019

No rush, we appreciate your work on this. As this is likely a post-v1 improvement, you have plenty of time.

@3cp
Copy link
Member

3cp commented May 23, 2019

BTW, please follow the commit message convention to be properly logged in changelog and release note. https://github.com/DurandalProject/about/blob/master/CONTRIBUTING.md#commit

@Sayan751
Copy link
Member Author

Missed the git commit message guideline clearly. Will adhere to that in following commits.

@3cp
Copy link
Member

3cp commented May 23, 2019

You can squash all commits into one if you want, that can avoid some similar entries from multiple commits in changelog.

Sayan751 added 4 commits May 23, 2019 16:37
Usage of aurelia-cli is removed from building and running
the webpack apps. This gives developers more control over
webpack, and a way to easily change the configuration,
without aurelia-cli meddling in between.

The commands `au run` or `au build` will not work anymore
in the webpack apps, use `npm start`, or `npm run build`
instead. This commit closes the webpack part of the
the issue aurelia#1098.
@Sayan751
Copy link
Member Author

@3cp @fkleuver It is ready for review again.

@Sayan751
Copy link
Member Author

Just wanted to point out that this pull request sort of addresses #1039 as well.

@3cp
Copy link
Member

3cp commented May 28, 2019

FYI, it's not <feat>(topic):, it's just feat(topic):.

@alexdresko
Copy link

bump

@fkleuver
Copy link
Member

@Sayan751 can you rewrite that one commit message with feat to follow conventional commit guidelines please? only then will it correctly show up in generated changelogs
I can't really comment much on the contents, @3cp what are your thoughts at this point?

@3cp
Copy link
Member

3cp commented Jul 12, 2019

We need to polish it up, and have some kind of plan to handle this kind of breaking change.

We (Sayan included) are all bit busy with Aurelia 2 😅 , not spending enough energy here.

@Sayan751
Copy link
Member Author

@fkleuver @3cp I can try to squash all the commits to one with conventional commit message and force push, if that is ok?

@3cp
Copy link
Member

3cp commented Jul 12, 2019

Force push is fine.

@Sayan751
Copy link
Member Author

@fkleuver @3cp I tried to squash all the changes, but there are many conflicting changes. These changes have got pretty old and the fork got also out of sync. I think at this point it is best to create a new PR. I have started another branch that contains all the changes squashed into one commit that includes your suggestions. That branch is also synced with the Aurelia master. Currently I am running the release checks, once that is done, I would like to create a new PR and close this one. Let me if you think otherwise.

@Sayan751
Copy link
Member Author

Closed in favor of #1137

@Sayan751 Sayan751 closed this Jul 13, 2019
@alexdresko
Copy link

So can this be used now?

@Sayan751
Copy link
Member Author

Sorry @alexdresko you need to wait till #1137 is merged and published.

@alexdresko
Copy link

@Sayan751 Okay now can we use this? :)

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.

[Proposal] CLI-less webpack build
4 participants