Skip to content
This repository has been archived by the owner on Apr 8, 2020. It is now read-only.

Update aspnet-webpack-react to React 15 #256

Closed
wants to merge 5 commits into from

Conversation

geirsagberg
Copy link
Contributor

Fixes #255.

@dnfclas
Copy link

dnfclas commented Aug 12, 2016

Hi @geirsagberg, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

@dnfclas
Copy link

dnfclas commented Aug 12, 2016

@geirsagberg, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

@SteveSandersonMS
Copy link
Member

Thanks for this!

Is the change to package.json the only one that's strictly needed (that appears to be the case at first glance)? I'm not sure what the index.d.ts and HotModuleReplacement.d.ts files are, so I'm guessing those changes don't strictly need to be merged. If they do, please clarify.

@geirsagberg
Copy link
Contributor Author

Only the package.json part is necessary yes. The type definitions caused a problem when building a second time, as they would be included as input files and therefore could not be written to. This is a separate issue from the version update, and also a problem with the other packages in the repo. I can separate the fixes into their own pull requests if you like, tomorrow.

@SteveSandersonMS
Copy link
Member

Thanks for clarifying.

No need to modify this pull request - I'll just tweak the package.json file directly since it's such a tiny change.

As for submitting a pull request containing the other bits, that's fine, but please provide more info on why it's necessary. Does it only become necessary because of something that's different in React 15 (and if so what)? Or have you always found it necessary? If the latter, I'd like to understand it better, because it hasn't been necessary in any scenario I've personally experienced. Thanks!

@geirsagberg
Copy link
Contributor Author

I noticed after running ./builds.sh a second time (or just running tsc in any one of the package folders, I would get errors like this:

error TS5055: Cannot write file './Prerendering.d.ts' because it would overwrite input file.
error TS5055: Cannot write file './index.d.ts' because it would overwrite input file.
npm WARN WebApplicationBasic@0.0.0 No repository field.
npm WARN WebApplicationBasic@0.0.0 No license field.
(node:99749) fs: re-evaluating native module sources is not supported. If you are using the graceful-fs module, please update it to a more recent version.
error TS5055: Cannot write file './ObjectAssignPolyfill.d.ts' because it would overwrite input file.
error TS5055: Cannot write file './StrongActions.d.ts' because it would overwrite input file.
error TS5055: Cannot write file './StrongProvide.d.ts' because it would overwrite input file.
error TS5055: Cannot write file './main.d.ts' because it would overwrite input file.

-> running update
error TS5055: Cannot write file 'dist/CachePrimedHttp.d.ts' because it would overwrite input file.
error TS5055: Cannot write file 'dist/Exports.d.ts' because it would overwrite input file.
error TS5055: Cannot write file 'dist/Validation.d.ts' because it would overwrite input file.
error TS5055: Cannot write file './fetch.d.ts' because it would overwrite input file.
error TS5055: Cannot write file './index.d.ts' because it would overwrite input file.
error TS5055: Cannot write file './main.d.ts' because it would overwrite input file.
npm WARN angular2-universal@0.104.5 requires a peer of parse5@^1.5.0 but none was installed.
npm WARN WebApplicationBasic@0.0.0 No repository field.
npm WARN WebApplicationBasic@0.0.0 No license field.

>> written zero files

npm WARN aspnet-prerendering@1.0.4 No repository field.
npm ERR! Darwin 15.6.0
npm ERR! argv "/Users/geirsag/.nvm/versions/node/v6.3.1/bin/node" "/Users/geirsag/.nvm/versions/node/v6.3.1/bin/npm" "install" "--quiet" "--depth" "0"
npm ERR! node v6.3.1
npm ERR! npm  v3.10.6
npm ERR! code ELIFECYCLE
npm ERR! aspnet-prerendering@1.0.4 prepublish: `tsd update && tsc && echo 'Finished building NPM package "aspnet-prerendering"'`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the aspnet-prerendering@1.0.4 prepublish script 'tsd update && tsc && echo 'Finished building NPM package "aspnet-prerendering"''.
npm ERR! Make sure you have the latest version of node.js and npm installed.
npm ERR! If you do, this is most likely a problem with the aspnet-prerendering package,
npm ERR! not with npm itself.
npm ERR! Tell the author that this fails on your system:
npm ERR!     tsd update && tsc && echo 'Finished building NPM package "aspnet-prerendering"'
npm ERR! You can get information on how to open an issue for this project with:
npm ERR!     npm bugs aspnet-prerendering
npm ERR! Or if that isn't available, you can get their info via:
npm ERR!     npm owner ls aspnet-prerendering
npm ERR! There is likely additional logging output above.

Excluding the .d.ts files from the input, makes this error go away. Of course it would be better to exclude the files by a glob such as *.d.ts, but I don't believe that is supported yet. (Maybe in TS 2.0?)

Apropos, I see now that there is a new pull request #258 to upgrade to TS 2.0, which might make this a non-issue?

@SteveSandersonMS
Copy link
Member

Oh I see. Thanks for clarifying. What we've done in other projects (e.g., aspnet-webpack) is to put a rimraf *.d.ts at the beginning of the prepublish script in package.json. I might add a similar thing to aspnet-webpack-react.

@geirsagberg
Copy link
Contributor Author

Ah yes, that would also do the trick :)

@geirsagberg
Copy link
Contributor Author

Hmm.. I just tested the ReactGrid example using the version of aspnet-webpack-react with React 15, and could not get it to work, so I'm closing this pull request for now, and will look into why it does not work.

* upstream/dev:
  Add ability to configure environment variables for Node instances, plus auto-populate NODE_ENV based on IHostingEnvironment when possible. Fixes aspnet#230
  Rename PrimeCache to PrimeCacheAsync (keeping older name as obsolete overload). Fixes aspnet#246.
  Prerenderer now passes original (unescaped) URL to Node - fixes aspnet#250
  In WebpackDevMiddleware, allow configuration of ProjectPath (implements aspnet#262)
@geirsagberg
Copy link
Contributor Author

Figured out the issue with ReactGrid; the problem was Griddle, not React 15 (I updated all npm dependencies to latest package version).

I have also added rimraf *.d.ts to all prepublish scripts, along with a few other fixes that kept build.sh from completing successfully.

@geirsagberg geirsagberg reopened this Aug 17, 2016
@dnfclas
Copy link

dnfclas commented Aug 17, 2016

Hi @geirsagberg, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

@@ -5,14 +5,16 @@ import { fakeData } from '../data/fakeData.js';
import { columnMeta } from '../data/columnMeta.jsx';
const resultsPerPage = 10;

const fakeDataWithAction = fakeData.map(data => Object.assign(data, {actions: ''}));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In latest version of Griddle, nothing is rendered for a cell if data does not have a property matching the column name, so had to add an empty field "actions".

@SteveSandersonMS
Copy link
Member

Excellent - looks great! Will merge soon. Thanks for this.

@@ -38,5 +38,5 @@ export interface Reducer<TState> extends Function {
}

export interface ActionCreatorGeneric<TState> extends Function {
(dispatch: Dispatch, getState: () => TState): any;
(dispatch: Dispatch<TState>, getState: () => TState): any;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch! Need this for my issue with TS2.0 PR & React as well.
node_modules/redux-typed/StrongActions.d.ts (16,16): error TS2314: Generic type 'Dispatch<S>' requires 1 type argument(s).

@geirsagberg
Copy link
Contributor Author

BTW I noticed a small issue with KoreBuild not building the sample projects correctly due to the sample projects being in a lower subdirectory (e.g. samples/react/ReactGrid/project.json), while KoreBuild uses the glob samples/*/project.json which only matches immediate child directories. I filed a pull request aspnet/KoreBuild#109 but they did not want to change the default (understandably).

I worked around the issue locally by changing the glob manually in the downloaded .build-files.

@MarkPieszak
Copy link
Contributor

Root dotnet restore also doesn't run webpack vendor files for each project which would make developing on the repo easier too, not sure if that's related!

@SteveSandersonMS
Copy link
Member

Thanks for cleaning up those parts of the build script too. This is now merged :)

@MarkPieszak
Copy link
Contributor

Hey @SteveSandersonMS I don't think this PR got merged somehow, it looks like none of the code is in the Repo (just tried searching for it, the old code is still there) Not sure why?

For example I need (dispatch: Dispatch<TState>, getState: () => TState): any; above for issue with TS2.0 PR & React as well.

node_modules/redux-typed/StrongActions.d.ts (16,16): error TS2314: Generic type 
    'Dispatch<S>' requires 1 type argument(s).

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

Successfully merging this pull request may close these issues.

4 participants