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

Add license headers in js/dist files #27308

Merged
merged 3 commits into from
Sep 26, 2018

Conversation

helmutgranda
Copy link
Contributor

@helmutgranda helmutgranda commented Sep 25, 2018

By implementing the same approach of rollup.config.js it can be replicated in build-plugins.js, with this approach individual plugins display license headers as expected.

Fixes #27301

@XhmikosR XhmikosR requested a review from Johann-S September 25, 2018 16:06
@XhmikosR XhmikosR changed the title Ad license headres in js/dist files Add license headres in js/dist files Sep 25, 2018
@XhmikosR
Copy link
Member

Thanks for the PR @helmutgranda!

@Johann-S do you think we could have the license header in a common place and use it in both scripts?

@helmutgranda
Copy link
Contributor Author

Thanks for the PR @helmutgranda!

@Johann-S do you think we could have the license header in a common place and use it in both scripts?

FYI, I thought of doing so but I didn't know if the PR/Review process would be more drastic but for sure that is a more elegant approach.

Copy link
Member

@Johann-S Johann-S left a comment

Choose a reason for hiding this comment

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

With the banner in a different file, I'll be good 👍

Thanks @helmutgranda 👍

Copy link
Member

@Johann-S Johann-S left a comment

Choose a reason for hiding this comment

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

With that changes it'll be good 👍

@@ -67,6 +68,7 @@ Object.keys(bsPlugins)
external
}).then((bundle) => {
bundle.write({
banner: banner,
Copy link
Member

Choose a reason for hiding this comment

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

banner, should be enough 😉

* Copyright 2011-${year} ${pkg.author}
* Licensed under MIT (https://github.com/twbs/bootstrap/blob/master/LICENSE)
*/`,
banner: banner,
Copy link
Member

Choose a reason for hiding this comment

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

Same here

@XhmikosR
Copy link
Member

I'm gonna test it tomorrow but I quickly made the changes.

@XhmikosR XhmikosR changed the title Add license headres in js/dist files Add license headers in js/dist files Sep 25, 2018
@XhmikosR
Copy link
Member

Works fine, thanks @helmutgranda!

@Johann-S: maybe we should add the component's name like we did in v3?

@Johann-S
Copy link
Member

Yep I think it'll work 👍

helmutgranda and others added 2 commits September 26, 2018 10:33
By implementing the same approach of rollup.config.js
and replicate it in build-plugins.js, individual plugins
will display license headers.
@@ -8,6 +8,7 @@
const rollup = require('rollup')
const path = require('path')
const babel = require('rollup-plugin-babel')
const banner = require(path.resolve(__dirname, './banner.js'))
Copy link
Member

Choose a reason for hiding this comment

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

Not a big fan of using path.resolve here because require('./banner.js') will works fine 🤔

Copy link
Member

Choose a reason for hiding this comment

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

True, let's change it

@XhmikosR XhmikosR merged commit 02c4444 into twbs:v4-dev Sep 26, 2018
@mdo mdo mentioned this pull request Sep 26, 2018
@helmutgranda helmutgranda deleted the add-license-headers branch September 26, 2018 14:43
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.

3 participants