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

Consider adding package.json + npm support #4

Closed
mathiasbynens opened this issue Feb 28, 2012 · 25 comments
Closed

Consider adding package.json + npm support #4

mathiasbynens opened this issue Feb 28, 2012 · 25 comments

Comments

@mathiasbynens
Copy link
Collaborator

This would take the burden off @walling, who maintains https://github.com/walling/xregexp.

We could write a simple script that concatenates all files together in the right order. It could be used as a post-commit hook.

The only thing that would need to change in the XRegExp source code is the way XRegExp is being exposed. @walling simply uses module.exports for this, which works fine in Node.js — but with just a few more lines we could support exporting to Narwhal, RingoJS, Rhino, and AMD loaders like RequireJS as well. I do this in Punycode.js as follows: https://github.com/bestiejs/punycode.js/blob/a6e30c4e2ce7a9a569bc2c84a3435bd5612be59f/punycode.js#L493-510

@slevithan
Copy link
Owner

Good call. +1 on adding package.json and npm support. And thanks for the pointer to punycode.js--I'll try to look over the referenced code and the related systems soon.

Do you think it's beneficial / appropriate to include all the addon files concatenated together with XRegExp? If you wouldn't mind writing the script for it, it would save me some research and prolly get done sooner since I need to mostly step away from this project for a couple weeks.

@walling
Copy link
Contributor

walling commented Feb 29, 2012

I can appreciate this move. You're welcome to steal the script I wrote, here: https://github.com/walling/xregexp/blob/master/package.sh I release it under MIT and 2-clause BSD license. :-)

You would need to add support for other loaders, but I think it should be quite easy as seen in punycode.js. I suggest making a new export.js that is appended to the other scripts and contains code to support other loaders.

When you're ready we can move the NPM package to your NPM user, so you can publish the new versions when released.

@mathiasbynens
Copy link
Collaborator Author

Rather than the export.js approach, I’d personally recommend use the Punycode.js-like export mechanism in xregexp.js by default. This way, there’s only one version of the main XRegExp file, and it will work in any environment.

Then, @walling’s script could be used to concatenate the files together. I’m not sure how exporting the plugins should work though… Perhaps @jdalton or @unscriptable know?

@walling
Copy link
Contributor

walling commented Feb 29, 2012

Nice approach, @mathiasbynens.

@slevithan
Copy link
Owner

Note that backcompat.js should not be included in the concatenated xregexp.js, primarily because of the changes planned for v2.0 that will stop XRegExp from adding to RegExp.prototype and modifying native methods by default (see the roadmap for details).

@jdalton
Copy link

jdalton commented Feb 29, 2012

@slevithan I was just gonna open an issue about making native RegExp.prototype extensions optional.... you might try an install() API name. MooTools is moving to it and I use it for an html5shiv rewrite (e.g. html5.install('styles methods'))

@slevithan
Copy link
Owner

@jdalton Great suggestion. Commit a94c925 brings an XRegExp.install placeholder. I've also updated README.md to use it, and outlined plans (in the Roadmap) for supporting XRegExp.install('natives'), XRegExp.install('methods'), and XRegExp.install('natives methods') in XRegExp 2.0.

Can you point me to information about MooTool's install?

@jdalton
Copy link

jdalton commented Mar 2, 2012

@slevithan Sorry for the delay.... here is the links to Moo2 (Milk.js):

https://github.com/mootools/Milk
https://github.com/mootools/Base/blob/master/Source/Host.js#L37-40

@jdalton
Copy link

jdalton commented Mar 2, 2012

@slevithan here is a small Milk.js example with its generic methods and then installing them to the Array.prototype

define(['Base/Utility/Array'], function(Array_) {
  // ..
  // invokes `myMethod` on each object in the array
  Array_.invoke([obj1, obj2, obj3], 'myMethod');

  // dump these methods to `Array.prototype`
  Array_.install();
  [obj1, obj2, obj3].invoke('myMethod');
});

@jdalton
Copy link

jdalton commented Mar 2, 2012

@slevithan Here is my html5.js usage:

// install support extensions with an options object
html5.install({

  // allow IE6 to use CSS expressions to support `[hidden]`
  // and `audio[controls]` styles
  'expressions': true,

  // overwrite the document's `createElement` and `createDocumentFragment`
  // methods with `html5.createElement` and `html5.createDocumentFragment` equivalents.
  'methods': true,

  // add support for printing HTML5 elements
  'print': true,

  // add default HTML5 element styles
  // (optional if `expressions` option is truthy)
  'styles': true
});

// or with an options string
html5.install('print styles');

// or using a shortcut to install all support extensions
html5.install('all');

I also have html5.uninstall(...) to undo what was installed.

@slevithan
Copy link
Owner

I've made several commits to address the two separate issues discussed here.

@jdalton Thanks for the examples and the pretty API! XRegExp now has install, uninstall, and isInstalled functions. install and uninstall take an options object or a whitespace/comma-delimited options string (I also copied the use of 'all'). isInstalled takes a string with a single option name only. XRegExp now has three optional/removable features: 'natives', 'methods', and 'extensibility'.

@mathiasbynens @walling I've added exports.XRegExp = XRegExp in the latest changes here. I'm basing this on QUnit's recently adopted solution, because I'm not very concerned about supporting all possible environments.

Is this not enough? Did I get something wrong? Which enviros will this specifically support or not support? Feedback is welcome, of course.

@walling
Copy link
Contributor

walling commented Mar 5, 2012

I prefer module.exports, so I'm able to do something like this: var XRegExp = require("xregexp") But seeing it's non-standard and not a very cross-platform solution I can definitely live with require("xregexp").XRegExp in my codebase. It's a matter of taste. I should just warn users in the readme, that this will change in the next NPM release. Otherwise your commit looks good.

I would also prefer that the NPM module is including all XRegExp addons by default. From the looks of package.json it seems only the core module is included. Are you willing to concatenate the modules (maybe using some automated script)? My reasoning behind this is that script load time doesn't matter much server-side, since the files are only loaded on startup, so why not include everything. This makes it easier to use the advanced features. Personally I use XRegExp mostly for Unicode matching and it would be a small annoyance to write that extra require statement every time I needed it.

What do you all think about this? I'm open for comments.

@jdalton
Copy link

jdalton commented Mar 6, 2012

@walling XRegExp could do both. Check out how Benchmark.js does it here:

  // expose Benchmark
  if (freeExports) {
    // in Node.js or RingoJS v0.8.0+
    if (typeof module == 'object' && module && module.exports == freeExports) {
      (module.exports = Benchmark).Benchmark = Benchmark;
    }
    // in Narwhal or RingoJS v0.7.0-
    else {
      freeExports.Benchmark = Benchmark;
    }
  }
  // via an AMD loader
  else if (freeDefine) {
    define('benchmark', function() {
      return Benchmark;
    });
  }
  // in a browser or Rhino
  else {
    // use square bracket notation so Closure Compiler won't munge `Benchmark`
    // http://code.google.com/closure/compiler/docs/api-tutorial3.html#export
    window['Benchmark'] = Benchmark;
  }

So in Node devs can do

var Benchmark = require(...);

But if they choose to use the more widely supported

var Benchmark = require(...).Benchmark;

it will work too.

@jdalton
Copy link

jdalton commented Mar 6, 2012

@slevithan

Thanks for the examples and the pretty API! XRegExp now has install, uninstall, and isInstalled functions. install and uninstall take an options object or a whitespace/comma-delimited options string (I also copied the use of 'all'). isInstalled takes a string with a single option name only. XRegExp now has three optional/removable features: 'natives', 'methods', and 'extensibility'.

Woot nice! I dig the isInstalled method too. I will bikeshed the use of isInstalled(...) vs. installed(...) a bit more on my end :D

@jdalton
Copy link

jdalton commented Mar 6, 2012

@slevithan I noticed you opt'ed to install all by default. I chose the opposite for html5.js because I noticed that the current html5shiv.js was causing problems with third party code because of it's automatic paving of natives. This way by default it won't cause problems and it's up to the devs to clobber stuff via install(...).

@slevithan
Copy link
Owner

@jdalton

@walling XRegExp could do both. Check out how Benchmark.js does it here

That seems very robust, but if it's feasible (i.e., if enough of the latest versions of the most popular loaders support a standard interface), I'd really like to support only the single most standard/popular method and keep the supporting code down to a minimum.

I dig the isInstalled method too. I will bikeshed the use of isInstalled(...) vs. installed(...) a bit more on my end :D

Yeah, I struggled with that one, too. In the end, I used isInstalled based on ES5's Array.isArray and for consistency with XRegExp's own XRegExp.isRegExp. However, I'm interested to see which way you end up going on this--it's not too late for me to change the name before the next tagged release.

I noticed you opt'ed to install all by default. I chose the opposite for html5.js

Yes, that's only for backward compatibility, since not installing natives automatically is a major breaking change. I plan to remove the automatic installation in XRegExp 2.0 (the next version after the forthcoming v1.6). But then, I've recently been leaning toward skipping 1.6 and jumping straight to 2.0, especially since the next release will have the backcompat.js file to smooth over such changes anyway.

. . . OK, it's decided--I'm jumping to v2.0. No installing optional features automatically. :-) The mounting change list warrants bumping the major number anyway.

@slevithan
Copy link
Owner

@walling

I would also prefer that the NPM module is including all XRegExp addons by default. From the looks of package.json it seems only the core module is included. Are you willing to concatenate the modules (maybe using some automated script)? My reasoning behind this is that script load time doesn't matter much server-side, since the files are only loaded on startup, so why not include everything. This makes it easier to use the advanced features.

Yes, I'm willing. Any chance you could write the patch for package.json and a script for concatenating the modules (perhaps to an xregexp-all.js file in the project's root directory)? Would this affect the exporting mechanism? I'd like to keep the exports.XRegExp line in xregexp.js.

Personally I use XRegExp mostly for Unicode matching and it would be a small annoyance to write that extra require statement every time I needed it.

Always glad to hear how it's used. You might be interested to know that the current HEAD version of Unicode Base (v1.0.0-dev) adds two major features vs v0.6: support for aliases (e.g., \p{UppercaseLetter} instead of \p{Lu}), and support for negated Unicode tokens within character classes (e.g., [\p{^..}]; there are no more restrictions on where Unicode tokens can be used).

@slevithan
Copy link
Owner

@jdalton FYI, commit 80d1243 removed automatic installation of optional features, and commit cd96f3d removed XRegExp's reliance on overriden native methods.

@walling
Copy link
Contributor

walling commented Mar 6, 2012

See the referenced pull request. By the way I'm looking forward to the Unicode updates. :-)

@walling
Copy link
Contributor

walling commented Mar 6, 2012

@slevithan, I made a notice on my NPM module GitHub project that people should switch to require('xregexp').XRegExp. Currently both are possible in the NPM module I just published. I took the liberty to use version number 1.5.2, because NPM needs increasing numbers.

See "If you already use this module" on https://github.com/walling/xregexp#readme.

@walling
Copy link
Contributor

walling commented Mar 6, 2012

I also wrote a notice for at least one known user of the NPM module: vjeux/jsMatch#2

@slevithan
Copy link
Owner

Cool cool. Merged. :-) See my question about tests/ in #7.

I'll leave this issue open for a few days in case anyone has further feedback.

@jdalton
Copy link

jdalton commented Mar 9, 2012

@slevithan btw I settled on isInstalled :D

@slevithan
Copy link
Owner

Good to know. I'm sticking with it, too. :)

@slevithan
Copy link
Owner

@jdalton and @walling

FYI, in commit b694fc5 I started using module.exports (look at tools/intro.js), so with the latest build you should be able to use either require('xregexp') or require('xregexp').XRegExp. :D

@walling In the same commit I made some tweaks to the node-qunit based testing. If you're able to confirm that it still works as expected, I'd really appreciate it, since I'm unable to run Node.js locally at the moment.

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

No branches or pull requests

4 participants