Skip to content

Make forkable#145

Merged
benjamn merged 4 commits intobenjamn:masterfrom
jamestalmage:make-forkable
Jul 11, 2016
Merged

Make forkable#145
benjamn merged 4 commits intobenjamn:masterfrom
jamestalmage:make-forkable

Conversation

@jamestalmage
Copy link
Copy Markdown
Contributor

@jamestalmage jamestalmage commented Apr 20, 2016 •
edited
Loading

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'))

@jamestalmage jamestalmage mentioned this pull request Apr 22, 2016
Closed
@benjamn
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

jamestalmage commented May 21, 2016

I would think so. I will update soon.

@hzoo
Copy link
Copy Markdown
Contributor

hzoo commented Jul 1, 2016 •
edited
Loading

@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
Merged
@jamestalmage
Copy link
Copy Markdown
Contributor Author

jamestalmage commented Jul 6, 2016

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
Copy Markdown
Contributor

hzoo commented Jul 6, 2016 •
edited
Loading

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
Copy Markdown
Owner

benjamn commented Jul 11, 2016

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

@jamestalmage
Copy link
Copy Markdown
Contributor Author

jamestalmage commented Jul 11, 2016

🎉 happy this made it in!

benjamn added a commit that referenced this pull request Aug 15, 2016
@benjamn
Bump NPM version to 0.9.0 in preparation for publishing.
65fd5d3 
The minor version bump is due to the significant code changes involved in
#145, even though no behavioral
changes are expected.
@bnjmnt4n bnjmnt4n mentioned this pull request Feb 6, 2019
Closed
@benjamn benjamn mentioned this pull request Jul 21, 2024
Open
schrabooth added a commit to schrabooth/ast-types that referenced this pull request Aug 4, 2024
@schrabooth @benjamn
Bump NPM version to 0.9.0 in preparation for publishing.
e503858 
The minor version bump is due to the significant code changes involved in
benjamn/ast-types#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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jamestalmage @benjamn @hzoo