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

[guide] Non-code issues: keep them out of your code files. #1643

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mk-pmb
Copy link

@mk-pmb mk-pmb commented Nov 22, 2017

Summary of #1640. I extrapolated the reasoning to other transport layer packaging, hope it's the right conclusion.

@mk-pmb mk-pmb force-pushed the explain-no-transport branch 3 times, most recently from c6d3199 to 0bc58d1 Compare November 23, 2017 09:30
@mk-pmb
Copy link
Author

mk-pmb commented Feb 9, 2018

rebased on current master.

Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

This section seems like it's full of "don't"s, without any "do's".

Can you be more specific about what kinds of things this advice would avoid, and what better alternatives would look like?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@mk-pmb
Copy link
Author

mk-pmb commented Feb 10, 2018

For clearer dos and don'ts about the BOM, we could use code blocks that contain just explanatory comments. Would at least keep the visual style consistent:

/* <- bad:  Invisible BOM at start of code file, meant to
            fix your editor's charset.
      good: Set the charset in your editor config.
            If you need per-project config and your editor
            doesn't support it, get a better one.           */
/* <- bad:  Invisible BOM at start of code file, meant to
            fix some browser's charset guessing.
      good: Use your webserver or some part of your toolchain
            to add the charset info.                        */

@mk-pmb
Copy link
Author

mk-pmb commented Feb 10, 2018

Draft of example for omitting potential UMD wrappers:

// bad: Bloat project code with AMD/UMD export.
(function unifiedExport(e) {
  var d = ((typeof define === 'function') && define),
    m = ((typeof module === 'object') && module);
  if (d && d.amd) { d(function () { return e; }); }
  if (m && m.exports) { m.exports = e; }
}(api));

// good: 
// Just omit it. Use your toolchain to add compatibility
// mechanisms only in the dist version.

Edit: Not a good example though, since an export statement would prevent old browsers that need AMD/UMD from parsing the script at all. Overlaps with rule 10.1.

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