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

Make forkable #145

Merged
merged 4 commits into from Jul 11, 2016
Merged

Make forkable #145

merged 4 commits into from Jul 11, 2016

Conversation

jamestalmage
Copy link
Contributor

@jamestalmage jamestalmage commented Apr 20, 2016

My attempt at fixing #57.

// all submodules now export a function that takes a `fork` instance
module.exports = function (fork) {

  // this use of fork.use replaces "require" everywhere.
  var types = fork.use(require('../lib/types')); 

  // modules that exported something should now return it from the function instead.
  return exportsObject;
};

Fully customizable definitions can now be achieved:

var types = require('ast-types/fork')([
  require('ast-types/def/core'); // just the core
]);

One potential downside (and I don't see it being a big deal):

var types1 = require('ast-types/fork')([
  require('ast-types/def/core');
]);

var types2 = require('ast-types/fork')([
  require('ast-types/def/core');
]);

// This throws with "[object Object] is not a Def"
// It probably should throw, but the error message needs improvement.
types1.Type.def('Node').isSupertypeOf(types2.Type.def('Expression'))

@benjamn
Copy link
Owner

benjamn commented May 21, 2016

Thanks for this @jamestalmage!

If we're going to change all that whitespace anyway, I would really love to finally switch to using two spaces per tab for indentation. Would that be easy to fix?

@jamestalmage
Copy link
Contributor Author

I would think so. I will update soon.

@hzoo
Copy link
Contributor

hzoo commented Jul 1, 2016

@jamestalmage Can you update with what needs to be done here? Looks like we would need this to progress on babel/babel#3561 to account for babel 6 node types

@hzoo hzoo mentioned this pull request Jul 4, 2016
@jamestalmage
Copy link
Contributor Author

Rebased, and eliminated some irrelevant style-only changes that were cluttering the diff:

Best viewed via https://github.com/benjamn/ast-types/pull/145/files?w=1

@benjamn - I never did change to two tab spaces, but now I am concerned doing so would create headaches for @hzoo rebasing #162 once this is merged.

Would you prefer I hold off on the whitespace changes for now? @hzoo?

@hzoo
Copy link
Contributor

hzoo commented Jul 6, 2016

Might make it easier to read but you can always use ?w=1 and in my case i'm only adding a new file so it's fine

@benjamn benjamn merged commit 826f55e into benjamn:master Jul 11, 2016
@benjamn
Copy link
Owner

benjamn commented Jul 11, 2016

Thanks again @jamestalmage, especially for your patience. 🍴 🎉

@jamestalmage
Copy link
Contributor Author

🎉 happy this made it in!

benjamn added a commit that referenced this pull request Aug 15, 2016
The minor version bump is due to the significant code changes involved in
#145, even though no behavioral
changes are expected.
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.

None yet

3 participants