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

Support for Meteor 1.4.2+ #11

Closed
wants to merge 7 commits into from
Closed

Conversation

abecks
Copy link

@abecks abecks commented Nov 16, 2016

Prefer dynamicHead API with fallback to old injection method. Adds support
for Meteor 1.4.2+

Prefer dynamicHead API with fallback to old injection method. Support
for Meteor 1.4.2+
@PEM--
Copy link

PEM-- commented Nov 17, 2016

@arunoda Is it OK for you to merge this PR from @abecks?

Copy link

@banjerluke banjerluke left a comment

Choose a reason for hiding this comment

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

lib/server.js should use 2 spaces for tabs to match original coding style. But otherwise, it looks good to me, and I have it running flawlessly in development and production. (Caveat: I'm not super familiar with Meteor internals so I wouldn't spot any sneaky edge cases.)

@mxs
Copy link

mxs commented Nov 24, 2016

nice work @abecks I tried your changes with meteor-react-router-ssr and it doesn't work as intended.

Other packages like meteor-react-router-ssr also uses connect's middleware and calls InjectData.pushData. And in this instance it is called after your middleware which req.dynamicHead += res._injectHtml is called.

The result is that the injected data which came after your middleware is never actually written to the boilerplate.

I've got a few hacky fixes for this but I thought better mention this here first.

@abecks
Copy link
Author

abecks commented Nov 24, 2016

Interesting. A better solution would be to make the request available inside the InjectData.pushData method, but I think that would require a change to Picker and FastRender aswell and would not be backwards compatible. I'm willing to setup a fork to do it though.

It would also be great if Meteor checked the response for dynamicHead as well as the request. It feels weird to me that it is on the request in the first place. I could try to get a PR into Meteor for this. Any thoughts?

Edit: @mxs I wonder if adding a loose dependency on meteor-react-router-ssr would fix the issue, since it may ensure that my fix's middleware is called after SSR's is added to the stack. Would appreciate your opinion on the matter as I don't use SSR.

@arunoda
Copy link
Member

arunoda commented Nov 26, 2016

@abecks can we take this PR?
If this breaks, meteor-react-router-ssr I assume that's a not a good thing.

How about this strategy: (Since I am leaving the project as well)

  • Could you release a fork of this with this support?
  • Then release a fork of fast-render with this support.
  • I'll redirect users to your repo.

@abecks
Copy link
Author

abecks commented Nov 28, 2016

@arunoda Sounds like a deal. I have to continue to support FastRender and InjectData for my production app anyway.

I've released staringatlights:inject-data, repo here: https://github.com/abecks/meteor-inject-data
As well as staringatlights:fast-render, repo here: https://github.com/abecks/meteor-fast-render

I might be interested in maintaining Picker as well, if no one has grabbed that one yet. Did FlowRouter get a maintainer?

By the way, thanks for all of your contributions to the Meteor community. There is a lot I could not have done without your packages.

Edit: meteor-react-router-ssr is probably broken on Meteor 1.4.2+ with Fast Render with or without this fix. It's likely I can fix it if I get a response from @mxs

@comus
Copy link

comus commented Nov 29, 2016

hi, @abecks
i have a simple example to show that why your solution not work for react router ssr
https://github.com/comus/why-meteor-react-router-ssr-not-work

just git clone & meteor npm install & meteor

i added some console.log into the code.
the result shows that your code run before meteor-react-router-ssr 's WebApp.connectHandlers.use, yes meteor-react-router-ssr use WebApp.connectHandlers.use too. and i think this is the main different compare with flowrouter-ssr.
here is the result i got.

inside picker - WebApp.rawConnectHandlers.use - begin
  inside FastRender - fastRenderRoutes.middleware - before FastRender.handleOnAllRoutes
    inside inject-data - InjectData.pushData - begin
      key: fast-render-data
      value: { collectionData: {}, subscriptions: {}, loginToken: undefined }
    inside inject-data - InjectData.pushData - end
  inside FastRender - fastRenderRoutes.middleware - after FastRender.handleOnAllRoutes
inside picker - WebApp.rawConnectHandlers.use - end

inside inject-data - WebApp.connectHandlers.use - begin
  res._injectPayload: { 'fast-render-data': { collectionData: {}, subscriptions: {}, loginToken: undefined } }
  res._injectHtml: <script type="text/inject-data">%7B%22fast-render-data%22%3A%7B%22collectionData%22%3A%7B%7D%2C%22subscriptions%22%3A%7B%7D%7D%7D</script>
  
  inside meteor-react-router-ssr - WebApp.connectHandlers.use - before sendSSRHtml
    inside meteor-react-router-ssr - generateSSRData - before InjectData.pushData
      inside inject-data - InjectData.pushData - begin
        key: fast-render-data
        value: { collectionData: { profile: [ [Object] ] },
                 subscriptions: { profile: { '[]': true } },
                 loginToken: undefined }
      inside inject-data - InjectData.pushData - end
    inside meteor-react-router-ssr - generateSSRData - after InjectData.pushData
  inside meteor-react-router-ssr - WebApp.connectHandlers.use - after sendSSRHtml
inside inject-data - WebApp.connectHandlers.use - end

yesterday, meteor-react-router-ssr author updated that his project is no longer in active maintenance.
you can see: https://github.com/thereactivestack-legacy/meteor-react-router-ssr

…ers.use`, including `reactrouter:react-router-ssr`
@abecks
Copy link
Author

abecks commented Nov 29, 2016

@comus I have released 2.0.3 of staringatlights:inject-data and updated my repository with a fix for reactrouter:react-router-ssr. Can you give it a shot? I was able to get your demo working with this fix.

Thank you for the demo app, by the way.

@comus
Copy link

comus commented Nov 30, 2016

@abecks it works on my demo.
but it is not work for some use case.
for example Telescope
Telescope run ReactRouterSSR.Run inside Meteor.startup.
it is useful that let other files or packages config the routes before Meteor.startup.
and then merge the routes in Meteor.startup and run ReactRouterSSR.Run

Meteor.startup(() => {  // <---------------------------

  Telescope.routes.add([
    {name:"posts.daily",    path:"daily",                 component:Telescope.components.PostsDaily},
    {name:"posts.single",   path:"posts/:_id(/:slug)",    component:Telescope.components.PostsSingle},
    {name:"users.single",   path:"users/:slug",           component:Telescope.components.UsersSingle},
    {name:"users.account",  path:"account",               component:Telescope.components.UsersAccount},
    {name:"resetPassword",  path:"reset-password/:token", component:Telescope.components.UsersResetPassword},
    {name:"users.edit",     path:"users/:slug/edit",      component:Telescope.components.UsersAccount},
    {name:"app.notfound",   path:"*",                     component:Telescope.components.Error404},
  ]);

  const AppRoutes = {
    path: '/',
    component: Telescope.components.App,
    indexRoute: Telescope.routes.indexRoute,
    childRoutes: Telescope.routes.routes
  }

  ......

  ReactRouterSSR.Run(AppRoutes, clientOptions, serverOptions);
});

so, it will not work if i just modify my demo code with

Meteor.startup(function () {
  ReactRouterSSR.Run(AppRoutes);
});

@abecks
Copy link
Author

abecks commented Nov 30, 2016

I just published 2.0.4 of staringatlights:inject-data. @comus can you give that a shot? I've added a check to ensure that the injection middleware is always last.

@arunoda
Copy link
Member

arunoda commented Dec 6, 2016

@abecks Thanks for the work on this.
Since this project somehow related to FlowRouter and you are maintaining FastRender and willing to maintain Picker, I hope you should jump here.

I hope this is going to great. A lot of people want to help maintaining FlowRouter and related projects.

@abecks
Copy link
Author

abecks commented Dec 6, 2016

I'll introduce myself over there. I'm going to close this PR now as its against my master branch which has many more changes, including the package name. Can you redirect this repo to mine?

@abecks abecks closed this Dec 6, 2016
@arunoda
Copy link
Member

arunoda commented Dec 6, 2016

Could you send me PRs for both repos?
Add a note on the top of the README saying, this is not maintained anymore and add a link to your fork/repo.

@abecks
Copy link
Author

abecks commented Dec 6, 2016

Will do. Are you available at all for me to ask you questions from time to time regarding FastRender? I'm trying to diagnose an issue with the auth right now. How does FastRender accomplish the instant login feature?

@arunoda
Copy link
Member

arunoda commented Dec 6, 2016

Yes sure. I could answer.

How does FastRender accomplish the instant login feature?

FastRender saves the loginToken into the cookies. We do it with a client side code.
Then in the server, it reads that token(via cookies) and authenticate the related Fiber.

@abecks
Copy link
Author

abecks commented Dec 6, 2016

How does Meteor.user() become instantly available on the client? I have an issue where the FastRender payload contains the user object, but Meteor.user() is undefined until the Meteor's delayed authentication kicks in.

@arunoda
Copy link
Member

arunoda commented Dec 6, 2016

We add all the data we got to the collection via inject-data.
See: https://github.com/kadirahq/fast-render/blob/59ca6d499fde6a05c212a68296bb648ffe01c431/lib/client/fast_render.js#L45

@abecks
Copy link
Author

abecks commented Dec 6, 2016

How does Meteor know which user document is the one the login token is for? For example, lets say you were sending multiple users in the FastRender payload. How does it know which one to authenticate as? I was thinking it was using the login token, but its not present in the payload's user object.

Edit: Found what I was looking for https://github.com/meteor/meteor/blob/87681c8f166641c6c3e34958032a5a070aa2d11a/packages/accounts-base/localstorage_token.js#L117

@DenisGorbachev
Copy link

Please merge this PR! :)

@arunoda
Copy link
Member

arunoda commented Dec 12, 2016

@DenisGorbachev it's not possible due to compatibility issues. Check above.
Would you mind using @abecks's forks.

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

7 participants