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

Create UMD-style wrapper for PathUtils and add a package.json #1

Merged
merged 1 commit into from
Dec 21, 2015
Merged

Create UMD-style wrapper for PathUtils and add a package.json #1

merged 1 commit into from
Dec 21, 2015

Conversation

petetnt
Copy link
Contributor

@petetnt petetnt commented Dec 20, 2015

Hi @jblas,

there was some discussion about PathUtils in the Adobe/Brackets-project repo some time ago:
adobe/brackets#11726

PathUtils was eventually reverted as it was in Brackets, but as I was working on another feature this weekend I noticed that module wrapping PathUtils would still be useful for Brackets in the long run.

This PR does the following (small changes):

  • PathUtils is wrapped into a UMD returnExports-format wrapper, which means that it can be now included as
    • CommonJS style module with require("./path-utils")
    • AMD module with require(["path-utils"], function (PathUtils) { /***/ });
    • Browser global by just adding the file (<script src="path-utils.js"></script>)
      • The commit history looks a bit weird there as everything was indented 1 tab forward as a result, but only thing that has changed is the wrapper and how the module is exposed
  • Version was bumped from 0.1 to 0.0.2 (semantic version is needed for npm)
  • package.json was generated with npm init so that the module can be installed from GitHub with npm. If you wish to publish the module into npm ecosystem at some point, you can follow the instructions at [NPM docs - Publishing npm packages](https://docs.npmjs.com/getting-started/publishing-npm-packages].
    • The module is named jblas-path-utils as path-utils is already taken. Unless you release the module to npm, this doesn't matter much.
  • path-utils.js was uglified with UglifyJS
    • The copyright in the top was changed to a JSDoc @license field so that UglifyJS doesn't eat it.

If there's anything that you would like to change, something is wrong in the commit in general or you have some questions, I'll gladly follow up on it. If you want to land this on another branch or not at all, that's okay too 👍

module.exports = factory();
} else {
// Browser globals (root is window)
root.returnExports = factory();
Copy link
Owner

Choose a reason for hiding this comment

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

I think I would be ok with landing this PR in master if the non AMD/module.exports case defined a window.PathUtils global instead-of/or-in-addition-to the window.returnExports currently being used, making it backwards compatible with the original usage.

Out of curiosity, how is root.returnExports supposed to work in non AMD/module.exports pages/apps if it pulls in more than one JS library wrapped in this manner? Won't each successive module included in the page stomp over the previous value of returnExports?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jblas oh damn, my bad. I must have nuked over the last change after doing a git reset when I forgot to minimize the changes I made: the root.ReturnExports line is supposed to be root.PathUtils, which exposes PathUtils to window (root is this in browser, which and in browser window === this)

This has now been fixed in 6f466d6 (force push)

jblas added a commit that referenced this pull request Dec 21, 2015
Create UMD-style wrapper for PathUtils and add a package.json
@jblas jblas merged commit cc7f66e into jblas:master Dec 21, 2015
@petetnt
Copy link
Contributor Author

petetnt commented Dec 21, 2015

🎉

@jblas
Copy link
Owner

jblas commented Dec 21, 2015

@petetnt thanks for bringing this into the 'modern-world' for development. :-)

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