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.

Sign up for GitHub

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

Jump to bottom

Deprecate requireCordovaModule for non-Cordova modules #707

Merged
merged 4 commits into from
Sep 28, 2018
Merged

Deprecate requireCordovaModule for non-Cordova modules #707

merged 4 commits into from
Sep 28, 2018

Conversation

raphinesse
Copy link
Contributor

@raphinesse raphinesse commented Sep 28, 2018 •
edited
Loading

Addresses the deprecation part of #689.

@project-bot project-bot bot added this to new/unassigned/reopened prs in Apache Cordova: project-bot automation testing Sep 28, 2018
@project-bot project-bot bot added this to 🐣 New PR / Untriaged in Apache Cordova: Tooling Pull Requests Sep 28, 2018
@brodybits
Copy link
Contributor

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.

@brodybits
Copy link
Contributor

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

@raphinesse
Copy link
Contributor Author

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
@project-bot project-bot bot moved this from new/unassigned/reopened prs to reviewing prs in Apache Cordova: project-bot automation testing Sep 28, 2018
@brodybits brodybits mentioned this pull request Sep 28, 2018
Merged
@brodybits
Copy link
Contributor

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.

@janpio janpio moved this from 🐣 New PR / Untriaged to ⏳ Ready for Review in Apache Cordova: Tooling Pull Requests Sep 28, 2018
@raphinesse
Copy link
Contributor Author

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

@brodybits
Copy link
Contributor

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.

brodybits pushed a commit to brodybits/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

@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
Apache Cordova: Tooling Pull Requests automation moved this from ⏳ Ready for Review to 🏆 Merged, waiting for Release Sep 28, 2018
@project-bot project-bot bot moved this from reviewing prs to merged prs in Apache Cordova: project-bot automation testing Sep 28, 2018
@raphinesse raphinesse deleted the deprecate-requireCordovaModule branch September 28, 2018 23:38
brodybits pushed a commit to brodybits/cordova-lib that referenced this pull request Sep 30, 2018
@raphinesse
apacheGH-707 Fix for module names that start with "cordova-lib"
231c49a
brodybits pushed a commit to brodybits/cordova-lib that referenced this pull request Sep 30, 2018
@raphinesse
apacheGH-707 Emit deprecation warning when requiring non-cordova modules
64be468
@brodybits brodybits 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brodybits brodybits Awaiting requested review from brodybits

@dpogue dpogue Awaiting requested review from dpogue

Assignees
No one assigned
Labels
enhancement
Projects
Apache Cordova: Tooling Pull Requests
🏆 Merged, waiting for Release
Apache Cordova: project-bot automatio...
merged prs
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@raphinesse @brodybits