Skip to content
This repository has been archived by the owner on Feb 28, 2024. It is now read-only.

Residential Parking Form (out of box Knack.js form embed) #271

Closed
wants to merge 8 commits into from

Conversation

mateoclarke
Copy link
Contributor

@mateoclarke mateoclarke commented Oct 9, 2018

I've started trying to embed the Knack.js form/app for Residential Parking Permits ( https://atd.knack.com/parking ) in alpha. I thought it would be pretty straightforward since its just some 3rd party code and I followed the pattern that Simi set running the Recollect 3rd party js. Unfortunately I'm stuck on a bug and I'm looking for ideas...

Apparently the knack.js embed code is conflicting with React Static because it brings along an instance of babel/polyfill and React Static already has own instance. This error appears in the browser console and seems to prevent the script from executing: k.js:1 Uncaught Error: only one instance of @babel/polyfill is allowed

  • I'm not sure I can do anything to modify the Knack package (or how I would even try) to remove their instance of the babel/polyfill. Is there a way to remove babel/polyfill for this one page? :🤷‍♂️
  • The other thing I could try is to change the version of babel/polyfill that React Static uses. I think this would happen within webpack? not sure. Apparently (from my Stackoverflowing) idempotent-babel-polyfill can run multiple instances. https://github.com/codejamninja/idempotent-babel-polyfill
  • The easiest thing might just be to go back to the form.austin.gov subdomain idea like we considered for USFS. But that seems like a bummer since there will certainly be more vendor JS required for different features throughout Janis.

@mateoclarke
Copy link
Contributor Author

Providing a bit of an update here and also looping in @johnclary...

The first thing I tried with the guidance of @ifsimicoded & @briaguya was to update our version of Babel to v7 because they now support a noConflict mode where the error I was seeing gets downgraded to just a console warning instead of breaking the runtime. For more discussion on that decision, see their Github. I also updated React-Static to v6 since they use babel v7. Neither of these updates changed the error I was seeing.

I was able to get the Knack.js embed code working but only by disabling babel/polyfill altogether. This means, until we find a solution, the whole React-Static site would not be compatible with IE 11. I was able to do this a couple ways. One by removing it from our package.json dependencies entirely. Two, less extreme, by commeting out the code that adds babel/polyfill to the webpack configuration which happens here.

Once I was able to get the embedded Knack code running locally, I wanted to try a few things to see if I could get the babel/polyfill back. I explored using the idempotent-babel-polyfill library, but that made no difference and the error reemerged. I believe this is because, at the end of the day, Knack's code is emitting the error and no matter what version of babel we use, their code includes a version of babel <7 so there will be always be a conflict.

So, for demo purposes, we can proceed by leaving babel/polyfill removed from the site. But in order for this to be resolved, I think we have to get Knack to either update their code to use babel 7 which has the noConflict setting as a default. Or maybe they could provide a version of their knack.js file that doesn't include babel at all, but I'm not sure this wouldn't create more problems. I'm creating a support ticket with Knack to see how possible it might be for an upgrade to Babel v7 to occur on their end and what that timeframe might look like.

@mateoclarke
Copy link
Contributor Author

copy of message to Knack support:

Mateo Clarke
Oct 12, 8:20 PM EDT

Hi, I work for the City of Austin Transportation Dept. We're using a web framework called React-Static that depends on babel/polyfill to work on IE11. When we try to embed a Knack app into one of our pages, the knack.js code emits a warning (that stops code executation) saying we already have babel/polyfill.

We are wondering if there is a roadmap for your team to update knack.js embed code to use babel v7 which has noConflict enabled by default. This would allow us to run the knack code inside our web framework.

I'm attaching links to the unresolved Pull Request were I summarized this issue and to the discussion within the babel community about making the change to noConflict in v7:

Mateo Clarke

@johnclary
Copy link
Member

thanks @mateoclarke. oof, this one's a bummer. i don't have a whole lot of hope for knack upgrading to babel v7, but hopefully they get back to us soon. looking forward to the demo site and let's discuss further.

@mateoclarke
Copy link
Contributor Author

Response from Knack Support + my response:

Laura Theis (Knack)
Oct 15, 10:29 AM EDT

Hi Mateo - It looks like this is a topic we have discussed, but have not yet been able to make any adjustments on it. In the past when we've run into this, we've worked with customers to have them remove their references to babel/polyfill. Is that possible in your set up?

Thanks,
Laura


Hi Laura,

Thanks for your response and your willingness to support this issue. I'm also a little relieved to hear we aren't the only customer effected.

We've removed babel/polyfill from our webpack setup, yes. But this solution is only temporary because the framework (React-Static) we're using looses support for IE11 when you drop the polyfill. Being a government agency, we have stringent a11y requirements so dropping babel/polyfill isn't acceptable as a permanent solution.

Do y'all have any suggestions based on this information?

Many thanks!
Mateo

@mateoclarke
Copy link
Contributor Author

mateoclarke commented Oct 15, 2018

Laura Theis (Knack)
Oct 15, 2:06 PM EDT

Hi Mateo - We've actually only heard about it from one other customer on a smaller Knack plan and they didn't have the IE requirements so they were able to remove their reference to babel/polyfill. That was a couple of months ago and the last we heard of it so it hasn't been brought to the top of the priority fix list yet. 

But it sounds like you all have greater needs for this, so I'll get a review request into our engineering > queue on this topic. I'll keep you posted just as soon as I have any updated information for you.

@mateoclarke
Copy link
Contributor Author

I've started a new branch which doesn't change the versions of react-static or babel and focuses on small styling issues. Going to close out this issue in favor of #272

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants