Skip to content

Conversation

@devversion
Copy link
Contributor

@devversion devversion commented Apr 19, 2017

  • Now ships the typing files properly
  • No longer pushes the dist/ and types to the Git repository.
  • Changes all source-files from CRLF to LF (I know this makes the PR huge)

@DovydasNavickas Would it be possible to get a release for the fixed typings? We are trying to switch to the latest version of scss-bundle inside of Angular Material.

Fixes #25

* Now ships the typing files properly
* No longer pushes the dist/ and types to the Git repository.
* Changes all source-files from CRLF to LF
@DovydasNavickas
Copy link
Member

I've already shipped the scss-bundle@beta with .d.ts files added properly.

And for the pull-request.. Changing the line-endings in the same commit makes it impossible to review it, because every single line has been updated and seeing actual change is... Well, impossible.

Is there any particular reason you need LF endings, though?

dist and @types files are already published that way. Why do you need src files to be LF too?

https://github.com/SimplrJS/scss-bundle/blob/master/package.json#L10
And
https://github.com/SimplrJS/scss-bundle/blob/master/package.json#L13

@DovydasNavickas
Copy link
Member

And for publishing dist and @types into GitHub, there's a reason:
you can install your npm package from GitHub repo and even particular branch, which makes easier consumption of the package.

@devversion
Copy link
Contributor Author

@DovydasNavickas Here is a trick for reviewing such PRs:

https://github.com/SimplrJS/scss-bundle/pull/26?w=1

I changed the LF file-endings because I saw that you are trying to ship the library with LF line endings and my Git setup automatically fixes inconsistent line-endings (http://adaptivepatchwork.com/2012/03/01/mind-the-end-of-your-line/)

I see the point of that, and it makes sense. It's just not a best-practice to push built-stuff to the Github repository. As the name says, Source Control Management. There should be only source (http://alblue.bandlem.com/2011/03/git-tip-of-week-ignoring-build-output.html)

On Material 2 we do similar things. We just have a script that publishes the build artifacts into a different repository (just for builds - keeps the repo clean).

Anyway, thanks for releasing. I filed the PR because I wanted to help you. I feel like it' was not very welcomed.

@devversion devversion closed this Apr 19, 2017
@DovydasNavickas
Copy link
Member

It's not that it is not welcomed 😄 I've had already released the package with @types published seconds before your PR.
All of the help to make the package better is more than welcome. Thanks for the efforts, really! 👏

@devversion
Copy link
Contributor Author

devversion commented Apr 19, 2017

No problem. I know the PR might have felt opinionated, but it was really just to share some of the things that we have learned on Angular Material 1+2.

It's your project and your decision in how you want to ship stuff. Thanks again.

@DovydasNavickas
Copy link
Member

No problems! If you need anything else - fill the issues and we'll sort it out together! 😄

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Properly publish typings

2 participants