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

fix(ios): expose --source-maps CLI flag, default to source maps for non-production builds #10951

Merged
merged 5 commits into from
Jun 12, 2019

Conversation

sgtcoolguy
Copy link
Contributor

@sgtcoolguy sgtcoolguy commented Jun 7, 2019

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

Description:

  • Create a --source-maps CLI flag to force generation of source maps
  • fix --hide-error-controller to be a flag, not an option (internal refactoring, it's a flag used by test suite so we can continue despite errors)
  • Change source mapping behavior to behave like so:
    • if --source-maps flag is passed to CLI, turn them on
    • if tiapp.xml has a source-maps tag respect it's value
    • if neither, turn on source maps by default on non-'production' (deploy type) builds

@build
Copy link
Contributor

build commented Jun 7, 2019

Warnings
⚠️

iphone/cli/commands/_build.js#L5883 - iphone/cli/commands/_build.js line 5883 – Avoid calling back inside of a promise. (promise/no-callback-in-promise)

⚠️

iphone/cli/commands/_build.js#L6410 - iphone/cli/commands/build.js line 6410 – 'code' is defined but never used. Allowed unused args must match /^.+/. (no-unused-vars)

⚠️

iphone/cli/commands/_build.js#L6410 - iphone/cli/commands/build.js line 6410 – 'out' is defined but never used. Allowed unused args must match /^.+/. (no-unused-vars)

Messages
📖

💾 Here's the generated SDK zipfile.

📖

✅ All tests are passing
Nice one! All 3604 tests are passing.
(There are 464 tests skipped)

Generated by 🚫 dangerJS against 5cb1fbd

@sgtcoolguy
Copy link
Contributor Author

cc @brentonhouse Can you try this build? Does it help/fix the problems?

@sgtcoolguy
Copy link
Contributor Author

relates to tidev/node-titanium-sdk#106 and appcelerator/titanium_studio#1134

@sgtcoolguy sgtcoolguy changed the title fix(ios): turn off source mapping by default fix(ios): expose --source-maps CLI flag, default to source maps for non-production builds Jun 11, 2019
@jquick-axway
Copy link
Contributor

jquick-axway commented Jun 11, 2019

@sgtcoolguy, are you seeing any incremental build issues on iOS when enabling/disabling source-maps?

I was running into this issue on Android. The build directory's JS files were not being re-processed. This is because our ProcessJsTask's base class currently only process the JS files if 1 of the source JS files have changed. It doesn't check if the encryptJS or sourceMaps settings have changed since the last build. As a quick solution on Android, I've modified "_build.js" to do a full rebuild if these 2 settings have changed. It's a bit heavy handed, but it'll do for the moment... and I would expect these settings to rarely change too.
062194a

Edit:
When I do a build for the iOS simulator for the 2nd time, our new build/iphone/incremental directory gets deleted. I've tried deployment type "development" and "test". So, Jan's new JS incremental build system isn't working for me for some reason.

@sgtcoolguy
Copy link
Contributor Author

@jquick-axway I haven't looked into it deeper. I was doing ti clean between builds whenever I tweaked the logic in our source mapping (from tidev/node-titanium-sdk#106) because otherwise no JS files changed, and no setting changed so it wouldn't use my new logic for generating source maps.

It should be doing a full build if the jsanalyze options change: https://github.com/appcelerator/titanium_mobile/blob/master/cli/lib/tasks/process-js-task.js#L323-L327 which includes the sourceMaps options, but I don't see it checking about the this.encryptJS flag.

@sgtcoolguy sgtcoolguy merged commit 7feffd7 into tidev:master Jun 12, 2019
@sgtcoolguy sgtcoolguy deleted the TIMOB-27098 branch June 12, 2019 17:43
sgtcoolguy added a commit that referenced this pull request Jun 12, 2019
…10951)

* handle source-maps flag, then tiapp.xml value, then default to on for non-production
* move ios --hide-error-controller to flag from option
hansemannn pushed a commit to hansemannn/titanium_mobile that referenced this pull request Jul 8, 2019
…idev#10951)

* handle source-maps flag, then tiapp.xml value, then default to on for non-production
* move ios --hide-error-controller to flag from option
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

3 participants