JavaScriptCore support #18

ilyapuchka · 2017-02-04T20:23:15Z

This is an implementation of JavaScriptCore support, mentioned in krzysztofzablocki/Sourcery#135 that adds custom filters and tags.

ilyapuchka · 2017-02-17T19:56:42Z

@AliSoftware @djbe can some one review this?

djbe · 2017-02-17T19:59:12Z

Will check it out later in the weekend if I find some time. I've already seen the initial commits a while ago and those looked ok (from a quick glance).

The only thing I'm really really wary of is, what is the performance implication of this? I mean, it's JavaScript, that stuff by definition is slow as molasses 😄 (exaggerating, but you get my point?)

AliSoftware · 2017-02-17T20:03:08Z

Sorry for the delay @ilyapuchka , we're quite focused on finishing the Great Split (waiting for Kyle to release Stencil 0.8, hopefully tonight or early tomorrow) so we can finally (!) merge & freeze & tag & push everything, so we can have a clean setup again and release that long-awaited stable 4.2 version at last. That's why that PR hasn't been our priority 😉

Then we'll focus on repairing the CI and make it possible to ensure PRs don't break anything, so we can work on PRs serenely again, because our Travis setup is quite unstable and not trustworthy lately 😅

ilyapuchka · 2017-02-17T20:04:27Z

if it's ok for react native, I guess it will be ok for code generator tasks performance wise =)

djbe

I've gone through most of the code and added some comments. Some of these I haven't repeated for every single place where they apply, but you should get the gist of it.

Now before we start adressing these issues with the PR itself, could you please explain what your view is of how this will work?

As I understand it, script files need to be loaded manually using registerTag(...) or variations thereof. This is something that's either:

fixed in code (load a hardcoded set of javascript functions)
or through a mechanism that scans a (user given) directory for some files to be added.

Which of these did you have in mind, and where will they be added?

For the first case, if the list is hardcoded, then you might as well implement the tags natively in swift.
For the latter case, I'm then missing code for the mechanism that will automatically load and register these with the Stencil Environment.

To finish up, I'd just like to note that this PR is quite an interesting concept, with lots of potential for advanced users.

Edit: Also, please rebase on master to get the latest changes.

djbe · 2017-02-24T16:30:49Z

Sources/Environment.swift

@@ -25,9 +25,9 @@ public extension Extension {
  }
 }

-public func stencilSwiftEnvironment() -> Environment {
+public func stencilSwiftEnvironment(_ configure: (Extension) -> () = { _ in }) -> Environment {


Why is this needed? What's stopping you from doing the following:

let extension = Extension() extension.registerTag(...) let env = stencilSwiftEnvironment() env.extensions += [extension]

extensions is defined as immutable:

public struct Environment { public let templateClass: Template.Type public let extensions: [Extension]

Allright, so if it's immutable, modify this function to accept an existing list of extensions (default to empty array). Adding this closure to configure an extension just seems unnecessary and awkward.

djbe · 2017-02-24T16:33:52Z

Sources/JSEnvironment.swift

@@ -0,0 +1,332 @@
+//


Please split this up into some files separated by functionality. Maybe best to put these files in a subdirectory, but then don't forget that you'll have to modify the SPM (and podspec) definition + the code to deal with modules.

Moving JS wrappers to separate files (wrapper per file) will be ok?

To begin with, yeah, after that we'll see about moving it to a submodule. I can imagine some people using StencilSwiftKit on platforms where there's no JavascriptCore support.

djbe · 2017-02-24T16:34:25Z

Sources/JSEnvironment.swift

+import Stencil
+import JavaScriptCore
+
+public extension Extension {


Might be worth creating an Extension subclass instead of polluting the general Extension api.

Yes, that's what I did in a separate project, will apply those changes here.

djbe · 2017-02-24T16:41:48Z

Sources/JSEnvironment.swift

+    ///   return value.toUpperCase()
+    /// }
+    /// ```
+    public func registerFilter(_ name: String, script code: String, jsContext: JSContext = JSContext()!) {


Why is JSContext an IUO? swiftlint will probably fail on this.

djbe · 2017-02-24T16:51:17Z

Sources/JSEnvironment.swift

+
+extension JSContext {
+
+    public func registerStencilTypes() {


This should only happen once if possible, maybe guard check if the specific objects are already registered.

Also, why is this public?

moving to subclass of Extension will let it call this only once from initialiser

djbe · 2017-02-24T16:57:47Z

Sources/JSEnvironment.swift

+/// - Returns: result of block
+/// - Throws: JSException if JavaScript context has associated exception
+@discardableResult
+public func inJSContext(_ jsContext: JSContext, _ block: () -> JSValue?) throws -> JSValue? {


Why is this public? It's only used internally. Same applies to the functions below.

djbe · 2017-02-24T16:59:08Z

Sources/JSEnvironment.swift

+@objc protocol JSExportableTokenParser: JSExport {
+    func parse() -> [JSNode]
+    func nextToken() -> JSToken?
+    func compileFilter(_ token: String) -> JSResolvable!


djbe · 2017-02-24T17:00:07Z

Sources/JSEnvironment.swift

+    func parse() -> [JSNode] {
+        return bridgingError {
+            try parser.parse().map(JSNode.init)
+            } ?? []


Indentation

djbe · 2017-02-24T17:07:07Z

Tests/StencilSwiftKitTests/JSFiltersTests.swift

+
+class JSFiltersTests: XCTestCase {
+
+  func testJSFilter() {


Please rewrite the tests to be similar to the other node/filter tests. This means to write tests with a few parts:

given a list of tokens, parses a correct structure of nodes/filters and such

given a structure of nodes and a context, renders the correct output

some tests to verify that parsing/rendering correctly fails for invalid data.

ilyapuchka · 2017-02-25T00:01:30Z

@djbe My vision is exactly what you state as a second variant. Custom JS filters should be loaded by scanning some folder that user provides (with a command line option most likely). But it's up to the tool (consumer of this framework) to define how exactly it will be loaded. That's why it's not included in this PR as I believe its out of the scope of this framework. I will go through your comments latter. Thanks for the review!

ilyapuchka · 2017-03-19T18:50:40Z

@djbe I've addressed your comments and added more tests, please review my changes

ilyapuchka · 2017-04-08T16:44:04Z

@AliSoftware @djbe any more thoughts about this? 😉

djbe · 2017-04-11T11:09:38Z

Sorry, the last few weeks have been really busy for me. I'll take a look at your changes this weekend.

djbe

Some good changes, but still some work to do. We're getting there.

I still think it might be preferable to move this whole JSCore bit to a sub-module (spm) and sub-spec (cocoapods).

@AliSoftware any thoughts?

djbe · 2017-04-16T17:20:16Z

Sources/JSExtension.swift

@@ -0,0 +1,338 @@
+//


This is still one big file, please split this up as mentioned before, one file for each logical block. For example, move the JS wrappers to another file.

djbe · 2017-04-16T17:26:05Z

Sources/JSExtension.swift

+    ///  }
+    /// }
+    /// ```
+    public func registerTag(_ name: String, script code: String) {


Are all the wrappers below (such as JSTokenParser, JSToken, ...) needed for only this call? If so, we might want to re-think it. I'm not a fan of repeating and creating copies of everything in Stencil, just to support this. What if stencil adds new types, or changes how they work?

Maybe a more limited version would be preferable. For example, define a way that a javascript function/object can define a list of parameters and a render function. And then unify this with the "simple tag" you have above.

djbe · 2017-04-16T17:29:30Z

Tests/StencilSwiftKitTests/JSExtensionTests.swift

+
+class JSExtensionTests: XCTestCase {
+
+  func template(_ templateString: String,


You've added more tests, which is good, but you're still missing (mentioned before):

given a list of tokens, parses a correct structure of nodes/filters and such

given a structure of nodes and a context, renders the correct output

You're directly going from stencil template string to a render. We prefer to test this in 2 fases: parsing, and then rendering.

ilyapuchka · 2017-04-16T18:01:07Z

@djbe this code is already extracted into a framework - https://github.com/ilyapuchka/StencilJS so you can use it instead of merging this pr if you are interested in this feature. I don't think we will need this for Sourcery though, what was initial intention for this pr, as we already have EJS and Swift templates support which both look much more promising than Stencil.

djbe · 2017-04-16T18:17:59Z

Oh, ok then.

As you're no longer pushing for this feature, and unless anyone wants it, I'll consider this closed. We can always add StencilJS as a dependency at some point.

ilyapuchka added 2 commits February 4, 2017 21:22

feat: javascript filters

06b3dc4

feat: javascript simple tags

5a60c60

ilyapuchka changed the title ~~[WIP] JavaScriptCore support~~ JavaScriptCore support Feb 5, 2017

ilyapuchka added 4 commits February 5, 2017 13:19

feat: javascript tags

fe54404

bridging errors

63e17fc

some refactoring and fixes

881f026

Merge branch 'master' into features/javascriptcore

231f064

djbe requested changes Feb 24, 2017

View reviewed changes

ilyapuchka added 5 commits February 25, 2017 13:18

adressing code review comments

8600912

added more tests

76c07f1

Merge branch 'master' into features/javascriptcore

ad22229

fixed lint warnings

7ac0586

fix spm tests

8fd1f99

djbe requested changes Apr 16, 2017

View reviewed changes

djbe added status: WIP type: enhancement labels Apr 16, 2017

djbe added this to the Nice To Have milestone May 8, 2017

djbe closed this May 31, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JavaScriptCore support #18

JavaScriptCore support #18

ilyapuchka commented Feb 4, 2017 •

edited

ilyapuchka commented Feb 17, 2017

djbe commented Feb 17, 2017

AliSoftware commented Feb 17, 2017

ilyapuchka commented Feb 17, 2017

djbe left a comment •

edited

djbe Feb 24, 2017

ilyapuchka Feb 25, 2017

djbe Feb 25, 2017

djbe Feb 24, 2017

ilyapuchka Feb 25, 2017

djbe Feb 25, 2017

djbe Feb 24, 2017

ilyapuchka Feb 25, 2017

djbe Feb 24, 2017

djbe Feb 24, 2017

djbe Feb 24, 2017

ilyapuchka Feb 25, 2017 •

edited

djbe Feb 24, 2017

djbe Feb 24, 2017

djbe Feb 24, 2017

djbe Feb 24, 2017

ilyapuchka commented Feb 25, 2017 •

edited

ilyapuchka commented Mar 19, 2017

ilyapuchka commented Apr 8, 2017

djbe commented Apr 11, 2017

djbe left a comment

djbe Apr 16, 2017

djbe Apr 16, 2017

djbe Apr 16, 2017

ilyapuchka commented Apr 16, 2017

djbe commented Apr 16, 2017


		class JSExtensionTests: XCTestCase {

		func template(_ templateString: String,

JavaScriptCore support #18

JavaScriptCore support #18

Conversation

ilyapuchka commented Feb 4, 2017 • edited

ilyapuchka commented Feb 17, 2017

djbe commented Feb 17, 2017

AliSoftware commented Feb 17, 2017

ilyapuchka commented Feb 17, 2017

djbe left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilyapuchka Feb 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilyapuchka commented Feb 25, 2017 • edited

ilyapuchka commented Mar 19, 2017

ilyapuchka commented Apr 8, 2017

djbe commented Apr 11, 2017

djbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilyapuchka commented Apr 16, 2017

djbe commented Apr 16, 2017

ilyapuchka commented Feb 4, 2017 •

edited

djbe left a comment •

edited

ilyapuchka Feb 25, 2017 •

edited

ilyapuchka commented Feb 25, 2017 •

edited