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

Conversation

DanHarman
Copy link
Contributor

@DanHarman DanHarman commented Mar 9, 2017

Addressing issue #739

  • Added 'babel-plugin-transform-react-jsx-source' to development build
    to add line numbers to debug output.
  • Added 'cacheDirectory: true' to babel config which improves webpack
    build time c. 40%.

- Added 'babel-plugin-transform-react-jsx-source' to development build
to add line numbers to debug output.
- Added 'cacheDirectory: true' to babel config which improves webpack
build time c. 40%.
@dnfclas
Copy link

dnfclas commented Mar 9, 2017

@DanHarman,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla2.dotnetfoundation.org.

It will cover your contributions to all Microsoft-managed open source projects.
Thanks,
.NET Foundation Pull Request Bot

@DanHarman DanHarman changed the title Babel config enhancements #739 React Redux: Babel config enhancements #739 Mar 9, 2017
@dnfclas
Copy link

dnfclas commented Mar 9, 2017

@DanHarman, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, .NET Foundation Pull Request Bot

@DanHarman
Copy link
Contributor Author

I guess it might be nice to apply this to the non-redux template too? If you would like me to do so, I can add that in too.

One question I have is how do you go about testing these changes in situ of the repo? I tested my mods in a project I created from the published template, but I'm not sure how I would go about doing this directly from the repo? (e.g. I can't run webpack in the repo as various files are missing in the template folder that webpack is expecting).

@DanHarman DanHarman changed the title React Redux: Babel config enhancements #739 React Redux: Babel config enhancements Mar 9, 2017
@SteveSandersonMS
Copy link
Member

Thanks for this!

I guess it might be nice to apply this to the non-redux template too? If you would like me to do so, I can add that in too.

That would be great. I'll wait for an update then before merging this PR.

One question I have is how do you go about testing these changes in situ of the repo?

You can run the template projects directly. I don't know what missing files you encountered, but the following works:

cd templates/ReactReduxSpa
npm install
webpack --config webpack.config.vendor.js
webpack
dotnet restore
dotnet run

Optionally, also run set ASPNETCORE_ENVIRONMENT=Development (on Windows) or export ASPNETCORE_ENVIRONMENT=Development (on macOS/Linux) before dotnet run if you want HMR etc.

@DanHarman
Copy link
Contributor Author

DanHarman commented Mar 11, 2017

Ok fixed up reactspa too.

One thing I noticed is that all the npm packages are dev dependencies in packages.config. Is that right for jquery - thought that would be a client side library?

@SteveSandersonMS
Copy link
Member

As far as I can tell, if we wire up babel-plugin-transform-react-jsx-source like this, it produces incorrect line numbers in the warnings. I guess the line numbers it uses refer to some intermediate .jsx representation, not the .tsx files.

As per @gaearon's comment, the line numbers come from Babel. I don't know if it's possible to get the original line numbers to flow through somehow (e.g., using the data from sourcemaps).

@DanHarman If you have any thoughts on how it can be made to work correctly, please let me know! If not, we'll probably have to not introduce this (since wrong line numbers would be worse than no line numbers).

@SteveSandersonMS
Copy link
Member

@DanHarman Since the consensus is that there isn't currently a way to get correct line numbers when the original source is tsx rather than jsx, I suggest we close this. Do you agree?

@DanHarman
Copy link
Contributor Author

Yes. It's a shame they aren't accurate as still useful as a clue, but not good enough to ship to people I agree.

One thing though, the cacheDirectory option I also added is worth keeping as it does significantly improve the webpack build time - unless you found issues with that? If you are happy with it I can create a separate PR with just that in it.

@SteveSandersonMS
Copy link
Member

One thing though, the cacheDirectory option I also added is worth keeping as it does significantly improve the webpack build time

Yes, that would be great! I'll close this PR, but will be happy to merge a new one that just has the cacheDirectory bit in it. Thanks.

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.

3 participants