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

Replace underscore with lodash, update backbone to 1.3.3 #842

Closed
wants to merge 7 commits into from

Conversation

sgtcoolguy
Copy link
Contributor

@sgtcoolguy sgtcoolguy commented Aug 22, 2017

JIRA: Related to https://jira.appcelerator.org/browse/TIMOB-24980
https://jira.appcelerator.org/browse/ALOY-1168

Description:
I originally planned to use Lodash's mergeWith to implement TIMOB-24980, but we use underscore. I can bring in lodash's mergeWith special for this case, and have in a separate PR #843

While this works, I think it'd be generally helpful to update our 3rd-party library here.

So, this PR proposes we:

  • Add lodash 4.17.4
  • Deprecate underscore.js
  • Add Backbone 1.3.3, modify it to use lodash in place of underscore
  • Set default backbone version to 1.3.3
  • Use lodash's _.mergeWith to deeply merge the source mapper options and retain any user plugins/presets.

However, this is likely a breaking change, as there have been changes in behavior/method naming from underscore to loads, and we expose the library used as the _ global, so these changes propagate to the user's apps.

@sgtcoolguy sgtcoolguy changed the title Allow babel plugins to be added via config.json Replace underscore with lodash, update backbone to 1.3.3 Aug 22, 2017
@sgtcoolguy
Copy link
Contributor Author

I think if we do move this way we'll need to retain underscore (for older backbone usage), allow the user to opt in to using lodash, and possibly spit out a deprecation notice if using underscore or older backbone so we can migrate them forward. This is a helpful resource: https://github.com/lodash/lodash/wiki/Migrating

@brentonhouse
Copy link
Contributor

@sgtcoolguy -- I wonder if lodash could be included in a /node_modules/lodash directory so that a developer could simply run npm install lodash@latest (or another version) to easily change the version of lodash that is used with the app?

@feons
Copy link
Contributor

feons commented Oct 10, 2017

Why don't wee keep underscore or lodash as dev dependencies? So they stay under node_modules folder, and compiler codes don't need to require them like require('../../lib/alloy/lodash').

For that we also need to bundle another version of the modules under lib/alloy mainly for user apps.

@brentonhouse
Copy link
Contributor

That would separate the underscore for compiling from the underscore for Alloy runtime, correct?

@feons
Copy link
Contributor

feons commented Oct 10, 2017

@brentonhouse, yes that's right.

@hansemannn
Copy link
Contributor

@sgtcoolguy I've made sgtcoolguy#1 to resolve the merge conflicts.

@feons
Copy link
Contributor

feons commented Dec 12, 2017

I'm getting this error when compiling an app:

../alloy/Alloy/commands/compile/sourceMapper.js:69
	target.lines = getTextFromGenerator(target.templateContent, target.template).split(lineSplitter);

TypeError: getTextFromGenerator(...).split is not a function

@feons
Copy link
Contributor

feons commented Dec 12, 2017

According to migrating doc, _.template syntax is _.template(string, option)(data)

@feons
Copy link
Contributor

feons commented Dec 12, 2017

Now, jake test:all passed locally for me.

@feons
Copy link
Contributor

feons commented Dec 12, 2017

But we still need to verify the generated code for controller that uses databinding, specifically the _.template usage.

@sgtcoolguy
Copy link
Contributor Author

@feons I've merged titanium_mobile#9512, which means our current master branch is broken with alloy apps until we get this fix in (or disable some of the transpilation/hack the sdk).

I think it's important to move us forward here, though I realize it's not a great situation to force a breaking change to Alloy.

I'd love if anyone has any ideas how we may be able to hack babel's transpile to avoid this specific issue, cc @hansemannn @janvennemann @feons

Or did these PRs fix it? #868 and #870

@hansemannn
Copy link
Contributor

@sgtcoolguy I know it's not an ideal solution, but I am all in for this, even as a breaking change. And also pro calling it "Alloy 2.0" - it's about time. If developers want to use an old version, they can. And even if we need to push CLI to the next major for his, I'd still agree with that. Getting ES6/7/8 support is the most fundamental gap we are closing right now, so let's push on that.

@ewanharris
Copy link
Contributor

  • Backbone 1.3.3 was added as part of ALOY-1534
  • Moving to lodash is covered by ALOY-1168
  • I have filed ALOY-1651
  • From discussions we believe the use case that the mergeWith change aimed to achieve to now be possible

Based on the above I'm going to close this PR

@ewanharris ewanharris closed this Mar 14, 2019
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

5 participants