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

Add react server module tagger #300

Merged
merged 1 commit into from
Jun 29, 2016

Conversation

doug-wade
Copy link
Collaborator

@doug-wade doug-wade commented Jun 21, 2016

Continue breaking up #280 (see also #294 and #297)

@@ -13,6 +13,7 @@
"react-dom": "~0.14.2",
"react-server": "../react-server",
"react-server-cli": "../react-server-cli",
"react-server-module-tagger": "../react-server-module-tagger",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I make this transitive dependency a direct dependency so that react-server-module-tagger's prepublish script is called; otherwise, we get module not found errors from react-server-gulp-module-tagger and babel-plugin-react-server

@doug-wade doug-wade force-pushed the add-react-server-module-tagger branch from a34f964 to d5e97b9 Compare June 22, 2016 00:18
@doug-wade doug-wade force-pushed the add-react-server-module-tagger branch 2 times, most recently from af74ab3 to a92164d Compare June 24, 2016 21:36
@doug-wade doug-wade force-pushed the add-react-server-module-tagger branch from a92164d to 1a1587c Compare June 24, 2016 21:46
@doug-wade doug-wade added the enhancement New functionality. label Jun 24, 2016
var isWindows = ('win32' === process.platform);

/**
* A util function for tagging modules. Expects to find data on its prototype
Copy link
Contributor

Choose a reason for hiding this comment

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

It expects to find data in its context, right? Doesn't need to be on the prototype.

@roblg
Copy link
Member

roblg commented Jun 28, 2016

Looks reasonable to me, though I didn't go line by line through the stuff that was moved from react-server-gulp-module-tagger to react-server-module-tagger. 👍 to more documentation there.

@gigabo
Copy link
Contributor

gigabo commented Jun 28, 2016

Yeah, LGTM, too. 👍

I've got some interface shuffling itches, but nothing prohibitive.

@doug-wade doug-wade merged commit 8d0d033 into redfin:master Jun 29, 2016
davidalber pushed a commit to davidalber/react-server that referenced this pull request Jul 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants