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

added Element shims for SSR #43

Merged
merged 1 commit into from
May 7, 2019
Merged

added Element shims for SSR #43

merged 1 commit into from
May 7, 2019

Conversation

fjaskolski
Copy link
Member

Using Element breaks SSR (e.g. with Gatsby). We can simply shim Element to fix this.

Wasn't sure if we should move the shim to a separate file and import something like SSRSafeElement or leave it inline.

I stuck with inline as it's a bit more straightforward. @kamilmateusiak what's your preference on this?

@quarties
Copy link
Contributor

@fjaskolski as we discussed - I think we should prevent situations like this globally (i.e. by using some kind of wrapper). @Andarist, Filip said that you do not recommend a solution like this - could you elaborate why? I think it's a better option than fixing SSR compatibility inline per component as it requires contributors to remember about that.

@Andarist
Copy link
Member

Andarist commented Apr 5, 2019

Well, if you indeed have multiple places where you need to gaurd against this then you might create a helper prop type creator 🤷‍♂️ Keep in mind that it doesn't change much - each constributor would still have to know about this, so they would have to import that helper. So writing quickly inline isnt really much different from abstracting this away.

There isn't much you can do to truly ensure that it works in SSR environment other than running tests in server environment too or using custom babel/eslint plugins, where using babel to automate this might be too magical for some. Here you go anyway:

export default function({ types: t, template }) {
  const isPropTypesInstanceOf = t.buildMatchMemberExpression(
    "PropTypes.instanceOf"
  );
  const guardedElement = template.expression(
    `typeof Element !== "undefined" ? Element : ()=>{}`
  );
  return {
    visitor: {
      MemberExpression(path) {
        if (!isPropTypesInstanceOf(path.node)) {
          return;
        }

        if (!path.parentPath.isCallExpression()) {
          return;
        }

        const arg0 = path.parentPath.get("arguments.0");

        if (!arg0.isIdentifier() || arg0.node.name !== "Element") {
          return;
        }

        arg0.replaceWith(guardedElement());
      }
    }
  };
}

@fjaskolski
Copy link
Member Author

Well, if you ask me, such babel plugin does look a bit magical indeed 😄. And as @Andarist mentioned still requires the contributors to know about it.

@quarties @kamilmateusiak This issue becomes a little blocker for us. We'll probably need to go with a fork for now. How can we help in closing it?

@quarties
Copy link
Contributor

quarties commented May 7, 2019

Okay @fjaskolski, I'm approving this PR as it is a blocker for ya folks. We will figure it out inside of DS team. And @Andarist thanks for your insights 💪

@kamilmateusiak please let us know if you have any objections against this PR 🙏

@kamilmateusiak
Copy link
Contributor

fine by me, I'm gonna add the issue to our JIRA board and as @quarties said we will figure out a better solution later

@kamilmateusiak kamilmateusiak merged commit 9eb1f35 into master May 7, 2019
@quarties quarties deleted the feature/DS-67 branch June 19, 2019 10:30
dprzybylek pushed a commit that referenced this pull request Sep 24, 2019
dprzybylek pushed a commit that referenced this pull request Sep 27, 2019
dprzybylek pushed a commit that referenced this pull request Oct 14, 2019
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.

4 participants