Skip to content

Deprecate requireCordovaModule for non-Cordova modules#707

Merged
raphinesse merged 4 commits intoapache:masterfrom
raphinesse:deprecate-requireCordovaModule
Sep 28, 2018
Merged

Deprecate requireCordovaModule for non-Cordova modules#707
raphinesse merged 4 commits intoapache:masterfrom
raphinesse:deprecate-requireCordovaModule

Conversation

@raphinesse
Copy link
Contributor

@raphinesse raphinesse commented Sep 28, 2018 •
edited
Loading

Addresses the deprecation part of #689.

@brody4hire
Copy link

brody4hire commented Sep 28, 2018

I just pushed a couple updates that I think are needed:

  • emit deprecation warning event in case of use of requireCordovaModule with non-Cordova module
  • fix and update spec for requireCordovaModule

I am about to include the deprecation warning event in the hotfix proposed in #708.

@raphinesse
Copy link
Contributor Author

raphinesse commented Sep 28, 2018 •
edited
Loading

I just now wanted to push my implementation of this that I just finished. 😞

Would have been good if you let me know that you were working on this @brodybits.

@brody4hire
Copy link

brody4hire commented Sep 28, 2018

@raphinesse go ahead and push your implementation. I was only doing this to help me with the hotfix.

@raphinesse raphinesse force-pushed the deprecate-requireCordovaModule branch from 7ef7886 to 08458b2 Compare September 28, 2018 16:59
@raphinesse raphinesse force-pushed the deprecate-requireCordovaModule branch from 08458b2 to 74ffd92 Compare September 28, 2018 17:03
@raphinesse
Copy link
Contributor Author

raphinesse commented Sep 28, 2018

This version is not node@4 compatible, I'm afraid. But it should be straightforward to make it compatible.

@brodybits If you want to include this in #708, I think I can whip up a commit that makes this node@4 compatible.

@raphinesse raphinesse changed the title [WIP] Deprecate requireCordovaModule for non-Cordova modules [Deprecate requireCordovaModule for non-Cordova modules Sep 28, 2018
@raphinesse raphinesse changed the title [Deprecate requireCordovaModule for non-Cordova modules Deprecate requireCordovaModule for non-Cordova modules Sep 28, 2018
@brody4hire brody4hire mentioned this pull request Sep 28, 2018
Merged
@raphinesse raphinesse requested a review from dpogue September 28, 2018 18:18
@brody4hire
Copy link

brody4hire commented Sep 28, 2018

FYI I am only using part of 1fdc38d in patch PR #708. You can see it in 0b0ae7a. The other 3 commits so far are part of patch #708. Please let me know if you would like me to explain this in more detail.

@raphinesse
Copy link
Contributor Author

raphinesse commented Sep 28, 2018

@brodybits I noticed it and it's fine. The bug is fixed anyway.

@brody4hire
Copy link

brody4hire commented Sep 28, 2018

I hereby approve this PR with note of a nit that I can think of:

In case a user upgrades from Cordova 8, using requireCordovaModule with some modules will result in the warning message while using requireCordovaModule with some other modules will result in an installation error. Here are the modules I can find from Cordova 8 (8.0.0) that were dropped in master:

  • aliasify
  • dependency-ls
  • glob
  • nopt
  • opener
  • plist
  • properties-parser
  • request
  • shelljs
  • tar
  • unorm
  • valid-identifier
  • xcode

Another minor nit is that cordova-js is dropped in master. I suspect this should not be an issue in practice, just something we may want to be aware of.

brody4hire pushed a commit to brody4hire/cordova-lib that referenced this pull request Sep 28, 2018
@raphinesse
Context#requireCordovaModule unit tests
a3895c3 
(without test for warning messages as proposed in apacheGH-707 for master)
@raphinesse
Copy link
Contributor Author

raphinesse commented Sep 28, 2018

@brodybits Agreed. If we should fork off another 8.x branch from master we would need to apply something similar to #708 there too.

@raphinesse raphinesse merged commit 81439dc into apache:master Sep 28, 2018
@raphinesse raphinesse deleted the deprecate-requireCordovaModule branch September 28, 2018 23:38
brody4hire pushed a commit to brody4hire/cordova-lib that referenced this pull request Sep 30, 2018
@raphinesse
apacheGH-707 Fix for module names that start with "cordova-lib"
231c49a
brody4hire pushed a commit to brody4hire/cordova-lib that referenced this pull request Sep 30, 2018
@raphinesse
apacheGH-707 Emit deprecation warning when requiring non-cordova modules
64be468
@brody4hire brody4hire mentioned this pull request Oct 1, 2018
Closed
This was referenced Mar 25, 2019
Closed
Closed
Closed
@marcuspridham marcuspridham mentioned this pull request Mar 26, 2019
Closed
@mlegenhausen mlegenhausen mentioned this pull request Mar 27, 2019
Merged
@rj-david rj-david mentioned this pull request Mar 29, 2019
Closed
@josersleal josersleal mentioned this pull request Mar 29, 2019
Closed
@maxidirienzo maxidirienzo mentioned this pull request Mar 30, 2019
Closed
This was referenced Apr 16, 2019
Merged
Merged
@EmeryGuillaume EmeryGuillaume mentioned this pull request Apr 16, 2019
Closed
acran added a commit to BDSU/cordova-hot-code-push that referenced this pull request Dec 3, 2019
@acran
fix build compatibility for ios with cordova-lib@9+
40b5ba9 
The use of context.requireCordovaModule() for non-cordova modules was
deprecated in favor of node's require()
see apache/cordova-lib#707
@notsyncing notsyncing mentioned this pull request Aug 17, 2020
Open
staskuban added a commit to staskuban/cordova-SmartIDReader that referenced this pull request Sep 22, 2020
@staskuban
Update moveLicenceToProjectAndroid.js
2d75f00 
update require (apache/cordova-lib#707)
staskuban added a commit to staskuban/cordova-SmartIDReader that referenced this pull request Sep 22, 2020
@staskuban
Update setupProjectIOS.js
2842a38 
update require (apache/cordova-lib#707)
staskuban added a commit to staskuban/cordova-SmartIDReader that referenced this pull request Sep 22, 2020
@staskuban
Update moveLicenceToProjectIOS.js
fbcf946 
update require (apache/cordova-lib#707)
@Shatnik77 Shatnik77 mentioned this pull request Jun 2, 2021
Open
@mgagna mgagna mentioned this pull request Mar 10, 2025
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brody4hire brody4hire Awaiting requested review from brody4hire

@dpogue dpogue Awaiting requested review from dpogue

Assignees

No one assigned

Labels

enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@raphinesse @brody4hire