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

Webpack #346

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Webpack #346

wants to merge 4 commits into from

Conversation

mister-what
Copy link

Migration to Webpack and CommonJS compatibility

What issue is this PR resolving? Alternatively, please describe the bugfix/enhancement this PR aims to provide

Migrating build system of angular-loading-bar to Webpack. Furthermore making it usable as a CommonJS module from other Webpack projects or from node.js (via require()), while keeping compatibility to direct browser usage in <script> tags.

  • Migration of tests
  • Migration of bower.json
  • transformed CSS to SCSS for easier development (output is still CSS)
  • added npm scripts to package.json
  • updated dependencies
  • code review

Have you provided unit tests that either prove the bugfix or cover the enhancement?

Unit tests are still working after migration. This PR does not contain functional code changes.

Related issues

#320

@faceleg
Copy link
Collaborator

faceleg commented Mar 17, 2017

Looks like you put a lot of work into this and I'm behind the idea!

Question: is there a reason the built files are now minified?

@faceleg
Copy link
Collaborator

faceleg commented Mar 17, 2017

I'm not convinced moving to SCSS is a great idea.

If this project was moved to anything, I'd personally prefer to see it moved to PostCSS.

The CSS is so simple that I really don't see a need to migrate it to a pre/post processor. @chieffancypants thoughts?

package.json Outdated
@@ -1,6 +1,6 @@
{
"name": "angular-loading-bar",
"version": "0.9.0",
"version": "0.10.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please revert this version bump, if the PR is merged we'll bump the version as part of the release process (which will generate a git tag as well)

Copy link
Author

Choose a reason for hiding this comment

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

already done. See commit 0722a8e

package.json Outdated
"karma-coverage": "^0.1.0",
"angular-animate": "^1.6.3",
"angular-mocks": "^1.6.3",
"css-loader": "^0.27.3",
"grunt": "~0.4.1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is grunt still used?

test: /\.js$/,
exclude: /node_modules/,
loaders: [
'ng-annotate-loader'
Copy link
Collaborator

Choose a reason for hiding this comment

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

mixed ' and " in this file

Copy link
Author

Choose a reason for hiding this comment

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

I will take care for this issue.

@mister-what
Copy link
Author

I added the minification, because the build files are the thing that is used when using bower + wiredep in script tags. Maybe it makes more sense to have the minified core proper source mapped (eg. for debugging).

Reason to use SCSS is the automated vendor prefixing for rules. There were quite a lot of of vendor specific rules in the CSS. This hab bad impact on readability and is error prone.
Maybe a compromise is good at that place. Since CSS ist always valid SCSS, we can go back to form how it was before, but strip the vendor specific stuff.
I can take care for those changes later.
The reason for my change proposals is that we are migrating to webpack for the X-Force Exchange portal at the moment. We are using the loading bar and want to continue using it. But it does not go really well hand in hand with webpack. As I would guess, that we are not the only ones, having this problem, it makes sense to propose changes that don't impact the usage with script tags but allows using it as a commonjs module, too.

@mister-what
Copy link
Author

Code changes applied (see 7922c94)

@faceleg
Copy link
Collaborator

faceleg commented Mar 19, 2017

CSS

As someone who doesn't use SCSS anymore and ideally never will again, I'd rather see standard CSS or PostCSS used.

Leave it to you and @chieffancypants to decide.

Minification

I suppose you could output both a built vanilla and built minified version, people can choose.

Webpack

I use webpack too, importing like: import 'angular-loading-bar' and haven't had issues - what issues have you experienced?

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