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

/** vs /*! multiline comment style #308

Closed
revelt opened this issue Feb 28, 2019 · 14 comments
Closed

/** vs /*! multiline comment style #308

revelt opened this issue Feb 28, 2019 · 14 comments

Comments

@revelt
Copy link

revelt commented Feb 28, 2019

Very popular npm packages and projects use a different comment style, one which starts with /*!, with exclamation mark.

I wonder, why are we using /** instead of /*! and should we switch to the latter?

As far as I see /*! are used to prevent JS minifiers from stripping the block and many famous projects are using exactly this way. For example, Vue: https://unpkg.com/vue

@mjeanroy
Copy link
Owner

Hi @revelt,

This plugin use /** comment style but I am completely open to use /*! (in fact, I asked myself this question when I wrote this plugin).
For information, you can already use /*! if the template you are given starts with this, for example:

module.exports = {
  plugins: [
    license({
      banner: `/*! Copyright <%= moment().format('YYYY') %> */`,
    }),
  ],
}

Otherwise, it will add generate a comment block using /**.
I'm open to any suggestion, do you have ideas to handle this use case? We can add an option to choose comment style, what do you think?

@revelt
Copy link
Author

revelt commented Feb 28, 2019

I had a think about this, I'd say option is the most flexible way.

Plus, if we implement a dedicated option, you could strip all existing comment blocks from the banner and this way convert the style if any was present. This would allow us to support cases:

  • In messed up comment block banners (multiple openings, erroneous etc)
  • In missing tails of the comment block

What if we hungrily chomped any kinds of opening comment blocks from the beginning of the banner (considering those might be stacked with whitespace in between), then, same way chomp tail of the banner. Then, wrap with preferred style /*! or (default) /**?

What do you think?

Thanks for looking into this.

@revelt
Copy link
Author

revelt commented Jun 5, 2019

PS. I raised an issue in 3rd-Eden/commenting#8 to allow /*! blocks. If it was implemented there, we could simply pass the options flag there...

@mjeanroy
Copy link
Owner

mjeanroy commented Jun 5, 2019

I think, this can already be done with current version of commenting. We can probably just pass the desired comment style as an option directly in this plugin. Something like this:

const commenting = require('commenting');

const header = commenting(text.trim(), {
  extension: '.js',
  style: new commenting.Style(' *','/*!', ' */'),
});

The only question is: what do you suggest as option name (and I will be happy to implement that, or if you like, you can submit a PR)? :)

@revelt
Copy link
Author

revelt commented Jun 5, 2019

HM, that's a pickle. I did look around, I can't find a formal name how they're called. Maybe option could be something like ignoredCommentsStyle or genericCommentsStyle? Kangax mentioned that in http://perfectionkills.com/html-minifier-revisited/

@subz390
Copy link

subz390 commented Jun 30, 2019

Hi all, may I suggest that the user can define a commenting style in the banner object to extend the functionality of what we have now, or specify not using a style.

license({
  banner: {
    file: path.join(__dirname, 'HEAD'),
    commentingStyle: ['body', 'start', 'end'], // user defined style
    noCommenting: true, // no style used, prepend text as is
}),

Where commentingStyle is an Array then create a custom new style as your example @mjeanroy
Where noCommenting === true then omit styling and prepend the text as is.

Background: I prepend a Greasemonkey Meta Block. My use case works well with a banner file as a lodash template to insert version number script title, url links and so forth and already has // style comments.

If you like my proposal I could do some tests and if I'm happy with the results submit a PR for discussion.

@revelt
Copy link
Author

revelt commented Jun 30, 2019

^ very logical

@mjeanroy
Copy link
Owner

@revelt @subz390 I was not a big fan of having two options for more or less the same thing and I was not a big fan of giving an option to define the comment style like this. So, I added an option to specify the comment style using name linking to "pre-defined" style:

license({
  banner: {
    file: path.join(__dirname, 'HEAD'),
    commentStyle: 'regular', // the default, other options may be: `ignored', `slash` or `none`
}),

What this option mean:

  • When using regular comment style, the "classic" block is used (/** ... */).
  • When using ignored comment style (use the term that you suggested @revelt), the license block will be used (/*! ... */).
  • When using slash comment style, only slashed will be used (// ....).
  • When none is used, the banner is not modified at all (so it is up to the plugin user to be sure to have a valid comment block).

What do you think? Is that ok for you? Would you like to suggest an improvement?

@revelt
Copy link
Author

revelt commented Aug 1, 2019

Thank you, I'm happy 👍

@subz390
Copy link

subz390 commented Aug 1, 2019

Your suggestion gets my +1 vote, thank you 👍

@revelt
Copy link
Author

revelt commented Aug 1, 2019

I'm quite excited, updated my rollup configs already, waiting for release...

@mjeanroy
Copy link
Owner

mjeanroy commented Aug 1, 2019

@revelt @subz390 Version 0.10.0 has been published with this feature.

@revelt
Copy link
Author

revelt commented Aug 1, 2019

@mjeanroy how do we customise the comment style if banner is a string? For example, this does not work:

license({
	banner: licenseStr,
	commentStyle: "ignored"
})

I could stick the license string into a plain object and put commentStyle in that object but under which key does the licenseStr go then?

license({
	banner: {
		?: licenseStr,
		commentStyle: "ignored"
	},
})

thank you for help.

@subz390
Copy link

subz390 commented Aug 25, 2019

Just got around to updating my rollup.config.js to incorporate the above changes. Would like to say thank you very much @mjeanroy for implementing the options as discussed. It's a small thing, but now my headers are perfect and that makes my OCD feel all warm and fuzzy.
Also thank you for adding a depreciation warning note relating to banner.content.file mind blown - you'd think all developers could do something like that to help with smooth transitions and updates, instead of forcing your dependents to catch up only when things break.

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

No branches or pull requests

3 participants