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

Refactor fusion-plugin-react-router to make depending on browser history easier #738

Merged
merged 1 commit into from Apr 21, 2020

Conversation

btford
Copy link
Contributor

@btford btford commented Apr 18, 2020

Hello!

I'm creating this PR to follow up on a conversation I had with @ganemone.

tl;dr– I'd like to be able to access browser history in a provider of one of my plugins. Being able to access history in a provider saves me from writing a lot of boilerplate downstream, which greatly simplifies my application and plugin code.

I'm curious if the approach below is something that makes sense– I pulled apart the router plugin into distinct server and browser plugins, and then I have just the browser plugin depend on the router history.

Thanks in advance!

@CLAassistant
Copy link

CLAassistant commented Apr 18, 2020

CLA assistant check
All committers have signed the CLA.

@btford btford force-pushed the btford/feat/expose-browser-history branch 3 times, most recently from e1d53c5 to 86e7236 Compare April 20, 2020 19:56
@btford btford changed the title WIP: refactor fusion-plugin-react-router to make depending on browser history easier Refactor fusion-plugin-react-router to make depending on browser history easier Apr 20, 2020
@btford btford force-pushed the btford/feat/expose-browser-history branch 4 times, most recently from c6c1b58 to 28795b6 Compare April 20, 2020 22:46
@ganemone ganemone added the ci label Apr 20, 2020
@btford btford force-pushed the btford/feat/expose-browser-history branch from 28795b6 to 3f2c994 Compare April 20, 2020 23:38
@@ -174,6 +174,23 @@ app.register(GetStaticContextToken, (ctx: Context) => {
});
```

##### `BrowserHistoryToken`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ganemone – I made my best efforts here in terms of the docs. Let me know if you have feedback.

const simulator = setup(app);
await simulator.render('/');

expect(middlewareHistory).toBe(providerHistory);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mostly just want to assert that the instance is the same regardless of where you get it.

Is there anything else I should add to the tests?

Copy link
Contributor

Choose a reason for hiding this comment

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

this is good

ganemone
ganemone previously approved these changes Apr 20, 2020
lhorie
lhorie previously approved these changes Apr 21, 2020
@btford
Copy link
Contributor Author

btford commented Apr 21, 2020

Thanks @ganemone, @lhorie!

@lhorie
Copy link
Contributor

lhorie commented Apr 21, 2020

There appears to be a lint error, but other than that, LGTM

@btford
Copy link
Contributor Author

btford commented Apr 21, 2020

The lint error appears to be:


 
--
  | /monorepo/fusion-plugin-react-router/src/plugin.js
  | 70:45  error  'window' is not defined  cup/no-undef
  |  


...but this code was present before my change.

What is the recommended way to get ahold of window here? Or should I ignore/remove the warning?

@ganemone
Copy link
Contributor

The lint error appears to be:


 
--
  | /monorepo/fusion-plugin-react-router/src/plugin.js
  | 70:45  error  'window' is not defined  cup/no-undef
  |  

...but this code was present before my change.

What is the recommended way to get ahold of window here? Or should I ignore/remove the warning?

So the cup/no-undef rule is smart enough to recognize code that exists inside __NODE__ and __BROWSER__ blocks. On master, the usage of window was inside a if (__BROWSER__) block.

@btford
Copy link
Contributor Author

btford commented Apr 21, 2020

Fixed the lint. Does buildkite need to be manually restarted?

@ganemone
Copy link
Contributor

Fixed the lint. Does buildkite need to be manually restarted?

Are you sure you pushed your changes? It doesn't look like anything has been updated

@UberOpenSourceBot UberOpenSourceBot dismissed stale reviews from lhorie and ganemone via 6dc39e9 April 21, 2020 16:57
This makes it a bit easier to write plugins that depend on browser history.
ganemone added a commit that referenced this pull request Apr 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants