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

[TIMOB-26963] Remove fields added to iOS required modules #10929

Merged
merged 5 commits into from Sep 25, 2019

Conversation

whitfin
Copy link
Contributor

@whitfin whitfin commented May 31, 2019

JIRA: https://jira.appcelerator.org/browse/TIMOB-26963

These fields are being injected into iOS modules (in the case of the ticket, a JSON file). These fields seem redundant, so I just removed them. I assume Jenkins will break if they're necessary somehow :) I couldn't find them anywhere else at a glance.

@build build added this to the 8.1.0 milestone Jun 1, 2019
@build build requested a review from a team June 1, 2019 00:00
@build
Copy link
Contributor

build commented Jun 1, 2019

Warnings
⚠️ This PR has milestone set to 8.2.1, but the version defined in package.json is 8.3.0 Please either: - Update the milestone on the PR - Update the version in package.json - Hold the PR to be merged later after a release and version bump on this branch
⚠️

Commit 4150bdd2e98a04e32cf06632716731527977dd6e has a message "Remove fields added to iOS required modules" giving 2 errors:

  • subject may not be empty
  • type may not be empty
Messages
📖 👍 Hey!, You deleted more code than you added. That's awesome!
📖

💾 Here's the generated SDK zipfile.

📖

✅ All tests are passing
Nice one! All 3899 tests are passing.
(There are 475 skipped tests not included in that total)

📖

🚨 This PR has one or more commits with warnings/errors for commit messages not matching our configuration. You may want to squash merge this PR and edit the message to match our conventions, or ask the original developer to modify their history.

Generated by 🚫 dangerJS against 88099ff

@sgtcoolguy sgtcoolguy modified the milestones: 8.1.0, 8.2.0 Jun 3, 2019
Copy link
Contributor

@janvennemann janvennemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks to be a leftover of our old require implementation which, for some reason, assumed that every module needs to export an url and id property. I guess it's safe to be removed.

cc @sgtcoolguy to confirm since he did the rework on the require implementation

@sgtcoolguy
Copy link
Contributor

Ha I left this around because I had no idea how/if it was used. But hey, the tests all pass and the world didn't end. I'm fine with merging this for 8.2.1...

@sgtcoolguy sgtcoolguy modified the milestones: 8.2.0, 8.2.1 Sep 5, 2019
@brentonhouse
Copy link
Contributor

@sgtcoolguy - Is it possible this could get merged into 8.2.0?

@brentonhouse
Copy link
Contributor

I believe this PR will fix this issue as well: https://jira.appcelerator.org/browse/TIMOB-20487

@cb1kenobi
Copy link
Contributor

@brentonhouse It's too late to add this to 8.2.0. It'll have to go into 8.2.1.

@sgtcoolguy sgtcoolguy self-requested a review September 23, 2019 14:11
@ssekhri
Copy link

ssekhri commented Sep 23, 2019

FR Passed. Appropriate fields available for the required common js module.
Verified on:
Mac OS 10.14.6
Ti SDK: 8.3.0.v20190923144817
Appc CLI: 7.1.1
Node: 10.5.0
JDK: 10.0.2
XCode: 11.0
Studio: 5.1.4.201909061933
Device: iPhone 7Plus(v12.2), iOS simulator (v13.0)

@lokeshchdhry lokeshchdhry merged commit 4d96ddc into tidev:master Sep 25, 2019
sgtcoolguy pushed a commit to sgtcoolguy/titanium_mobile that referenced this pull request Sep 30, 2019
@whitfin whitfin deleted the TIMOB-26963 branch November 5, 2019 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants