search

deprecate / metal-uri

Class for parsing and formatting URIs.
Other
3 stars 7 forks source link

Throws an exception in ReactNative 16 #19

Closed ipeychev closed 6 years ago

ipeychev commented 6 years ago

In ReactNative 16 this check fails since URL is an object, but URL.length is 0.

Question: What if we drop the current implementation which uses URL or parsing via a element and replace it with a specially prepared module for that purpose, which has small footprint and does not use any native objects? This could be url-parse module.

eduardolundgren commented 6 years ago

How was that working before?

ipeychev commented 6 years ago

In ReactNative 15 URL was the native object as in the browser. In ReactNative 16 it is something else - their own class.

I opened an issue for them here to see what will they say too.

jbalsas commented 6 years ago

Can we wait until they let us know? We could simply adapt the native check if needed...

On the other hand, it is true that the native+fallback implementation we have is in fact more fragile and browser-dependent than we'd like to. Considering all the fixes we already had to apply for metal-uri to behave consistently across browsers, it might make sense to just go with a full-fledged urlparser.

ipeychev commented 6 years ago

Hey Chema,

I think we have to go ahead and change the implementation here for at least those reasons:

  1. Even if they answer us on time, which I doubt, it will take time to fix it and release a new version. Meanwhile, WeDeploy's API JS is unusable in ReactNative 16, probably in ReactVR, haven't tested in Electron, hope it works at least there.
  2. The current implementation is indeed fragile. Those are actually two implementations - Node and browsers and as I see a few patches were applied to keep the browser one in a good shape.
  3. Even with those two implementations, it doesn't work in all environments.

Eduardo is also OK if we change it and use only one implementation and we may start with url-parse. We have to keep the existing interface and change only the internals. It would simplify the module a lot. So, would it be possible for someone from Spain to apply this really quickly?

@jbalsas

jbalsas commented 6 years ago

Hey @ipeychev, we can definitely take a look, but I'm not so sure about the timeframe.

@vbence86 is finishing the update of AlloyEditor to React16, so he might be able to get to this as soon as he's done with it. @Robert-Frampton should be coming back from LSNA soon as well. It would take at least a couple of days, but we can prioritize this.

Just to clarify, are we talking about simply consuming url-parse or maintaining our own implementation?

ipeychev commented 6 years ago

Hey @jbalsas ,

I would say let's go for now with url-parse (or with something similar, I don't mind). I don't see a strong reason for our own implementation at this moment.

eduardolundgren commented 6 years ago

@jbalsas The url-parser module will replace the native URL and Node.js parsing. The rest of metal-uri will stay intact. This change, should not cause any changes in the test cases.

jbalsas commented 6 years ago

Yeah, we're on it, we'll get started as soon as we finish with the AlloyEditor React16 story I mentioned... unless @Robert-Frampton beats us to it 😉

robframpton commented 6 years ago

I have a branch with passing tests here.

jbalsas commented 6 years ago

This has been merged to develop and is scheduled to be released in 3.0.0 tomorrow! 🤞