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

Additional fixes for Redux #191

Merged
merged 8 commits into from Jun 14, 2016

Conversation

psfblair
Copy link
Contributor

@psfblair psfblair commented Jun 10, 2016

  • Had omitted react-router, which was needed for react-router-redux
  • Had neglected to update the .fsproj file with the new modules
  • Fixed some type issues in the generated declarations
  • Removed all setters -- Redux is designed such that all data is supposed to be immutable. Will revisit this if something breaks.
  • Documented version numbers of libraries that the interface declarations are based on
  • Included the original .ts files from which the interfaces were generated
  • Fixed some issues that were causing problems with electron (looks like one was also fixed on master, which is what is causing the conflict).

The brunch skeleton for Fable/Redux is on GitHub here: https://github.com/psfblair/fable_redux_skeleton -- however, it's not entirely ready yet (partly because issue #190 is blocking).

@alfonsogarciacaro
Copy link
Member

Nice work, thank you! The only thing I have some doubts is adding the .d.ts files to the repo. I'm no doing it for the other imports (though we could agree to do it from now on). So far I've been using tsd (I just found out that's been deprecated in favour of typings, see this) to manage typescript typings. This way you just need to commit a json file and we somehow prevent pollution of the repo, what do you think?

@7sharp9
Copy link
Collaborator

7sharp9 commented Jun 11, 2016

On 11 June 2016 at 10:24, Alfonso Garcia-Caro notifications@github.com
wrote:

.d.ts

Not sure I see the point unless the generator is driven by the .d.ts

@psfblair
Copy link
Contributor Author

Here's the rationale for including the .d.ts file -- the .fs interface declarations files are not simply autogenerated from the Typescript. They are initially generated, and then modified by hand. Including the original .ts file makes it possible to trace back what by-hand modifications needed to be made to the .fs file. This should make it easier to upgrade the interface declarations to later versions when new .d.ts files come out.

I was not aware of typings. I'll try working that in instead.

@7sharp9
Copy link
Collaborator

7sharp9 commented Jun 11, 2016

Just reference a link to where the d.ts comes from and what version, you can trawl the diff at any time in the future.

@alfonsogarciacaro
Copy link
Member

You're absolutely right, @psfblair, as the imports are manually edited it's important to track the diffs between the .d.ts versions so it's not necessary to repeat the whole process every time. Unfortunately not all the .d.ts files keep proper versioning and I myself haven't been very serious in this regard.

I have added a typings.json file to the repo and installed the typings corresponding to the currently imported definitions. You should be able to recover them by installing typings globally: npm install -g typings and then using typings install in Fable's directory.

You'll see that the typings.json file contains the source of each definition, its version and a commit hash. I'll add this info from now on on top of the Fable.Import files so it's easier to compare later the different versions of the d.ts files and see what has exactly changed 👍

@psfblair
Copy link
Contributor Author

Are you waiting for anything from me on this? I have typings, but I'm not entirely sure where the registry urls in typings.json came from. The only one that needs to be added is for react-router.

As I mentioned on a different issue, I'm afraid I'm not going to be able to carry this forward. I haven't successfully got Redux working, and there's some type problem with the way components are wired to the Redux state that I haven't been able to figure out. It's conceivable that this has to do with the interface definitions, but it seems more likely that it has to do with something going on under the hood.

@alfonsogarciacaro
Copy link
Member

I was waiting for your thoughts on whether the .d.ts files should go on the repo or not 😄 But if you're busy don't worry, I'll edit the PR myself 👍

@psfblair
Copy link
Contributor Author

I had removed the .d.ts files from the branch, but I did put a link into the READMEs which I can remove if you like. The main thing is that the react-router typing isn't part of the typings.json.

@alfonsogarciacaro alfonsogarciacaro merged commit 0b1b3b7 into fable-compiler:master Jun 14, 2016
@alfonsogarciacaro
Copy link
Member

Perfect, thanks for that! I just merged the PR and then removed history.d.ts which was still there :)

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