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

#6583 is breaking change - developers should know. #8046

Closed
5 of 6 tasks
mathertel opened this issue May 17, 2021 · 8 comments
Closed
5 of 6 tasks

#6583 is breaking change - developers should know. #8046

mathertel opened this issue May 17, 2021 · 8 comments
Milestone

Comments

@mathertel
Copy link
Contributor

Basic Infos

  • This issue complies with the issue POLICY doc.
  • I have read the documentation at readthedocs and the issue is not addressed there.
  • I have checked that the issue is present in current master branch (aka latest git).
  • I have searched the issue tracker for a similar issue.
  • If there is a stack dump, I have decoded it.
  • I have filled out all fields below.

The topic

There was a change should be listed under breaking changes as you have to re-write the parameter on any handler overriding the existing functions. If you have no override annotation it even does compile - but do not work.

Was introduced in #6583

As there is no example where a class is deriving from RequestHandler this is not detected in builds.

@Hieromon
Copy link

@earlephilhower, Is it possible to notify other library developers of such significant changes in advance?
You say RequestHandler is basically private, but I understood it was the public way to make the RequestHandler implementation. (That's why I used that utility)

@earlephilhower
Copy link
Collaborator

@Hieromon while it does look like this escaped notice as a breaking change, it was in the development repo for almost a full year before the 3.0.0 release, which is really the only way we have of letting the world know where the repo is going.

@mathertel we can add a note on the next bugfix about this, sure. Because this is a 2.x -> 3.x transition, breaking changes are expected, but looks like we missed a note about this.

@earlephilhower earlephilhower added this to the 3.0.1 milestone May 17, 2021
@mathertel
Copy link
Contributor Author

Thanks.

@mathertel
Copy link
Contributor Author

mathertel commented May 21, 2021

Here is a code snipped to declare the function so they can be implemented using the same code:

#if defined(ESP8266)
  bool handle(WebServer &server, HTTPMethod requestMethod, const String &requestUri) override
#elif defined(ESP32)
  bool handle(WebServer &server, HTTPMethod requestMethod, String requestUri) override
#endif

Same for upload, canHandle and canUpload

@Hieromon
Copy link

I reluctantly adopted the following implementation to ensure backward compatibility with my library. It's an incomplete method that relies on fixed member function signatures and is verbose if I try to write it completely.
Is there a better alternative to this way?

template<typename T>
struct TypeOfArgument;
template<typename T, typename U, typename... V>
struct TypeOfArgument<U(T::*)(V...)> {
  template<size_t i>
  struct arg {
    typedef typename std::tuple_element<i, std::tuple<V...>>::type  type;
  };
};

// Determines the type of the uri argument contained in the member function
// signature of the RequestHandler class. This redefinition procedure ensures
// backward compatibility with ESP8266 arduino core 3.0.0 and later.
// However, it relies solely on the canHandle member function as a criterion
// and lacks completeness.
using URI_TYPE_SIGNATURE = std::conditional<
  std::is_lvalue_reference<TypeOfArgument<decltype(&RequestHandler::canHandle)>::arg<1>::type>::value,
  const String&,
  String
>::type;

Class Foo : RequestHandler {
 public :
  Foo(){}
  ~Foo(){}
	
  bool canHandle(HTTPMethod requestMethod, URI_TYPE_SIGNATURE requestUri) override;
  bool canUpload(URI_TYPE_SIGNATURE uri) override;
  bool handle(WebServerClass& server, HTTPMethod requestMethod, URI_TYPE_SIGNATURE requestUri) override;
  void upload(WebServerClass& server, URI_TYPE_SIGNATURE requestUri, HTTPUpload& upload) override;
};

Initially, I tried to use esp8266::coreVersionNumeric, which is the recommended method for absorbing version, but constexpr parseNthInteger can not be used in the class member function declaration in order to be evaluated at the same timing as the instantiation of the template.

@d-a-v
Copy link
Collaborator

d-a-v commented May 22, 2021

Would a global define restoring the old-and-esp32-compatible API help ?
(something like #ifdef COMPAT_ESP32 or #if COMPAT_ESP32 >= 10001 // >= v1.00.01, you would have to use a global #define COMPAT_ESP32 10001 in your project)

@d-a-v
Copy link
Collaborator

d-a-v commented Jun 24, 2021

Closing this issue per following actions

@d-a-v d-a-v closed this as completed Jun 24, 2021
@Hieromon
Copy link

@d-a-v In conclusion, I find it difficult to completely eliminate the legacy method of relying on #define to satisfy static type determination in C++11 as a way to keep backward compatibility. It detains us.
These problems are common in infrastructure components that have been in use for a long time, but at the same time prove that they are valuable.
Anyway, thank you for your support.

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

No branches or pull requests

4 participants