cjss-group / CJSS

A CSS based web framework
https://cjss.js.org/
MIT License
670 stars 20 forks source link

Prepare functions instead of using safeEval #25

Closed c-harding closed 5 years ago

c-harding commented 5 years ago

As discussed in #7, eval is probably not needed in --data, and so I have converted this to JSON (backwards-compatible to also support JavaScript syntax of single-quoted strings, non-quoted keys and trailing commas). Elsewhere, I have replaced calls to safe_eval with the Function constructor, which means each function is only generated once, rather than once per matching element.

I have also changed the way that enclosing parentheses are parsed: instead of blindly discarding the first and last characters, this now only happens if they are actually a pair of parentheses (cjss.js:13).

scottkellum commented 5 years ago

👍 looks good to me

KargJonas commented 5 years ago

@scottkellum please wait with merging. I see a potential issue.

c-harding commented 5 years ago

@scottkellum please wait with merging. I see a potential issue.

@KargJonas what potential issue do you see? Was it fixed in 25f3d86?

KargJonas commented 5 years ago

I have two main concerns with your changes. 1) Implementing a json parser by hand with regex is a very bad idea. Maybe look at some prebuilt tools. 2) Code readability. safeEval was implemented with #14 in mind.

Make small changes in your PRs and I'll merge them one by one.

c-harding commented 5 years ago
  1. I can remove the JSON parser. I added it to allow backwards compatibility, but it probably introduces more problems than it solves. A similar library is JSON5, but importing a 50 kB library just for backwards compatibility seems like overkill. JSON5 may make a good plugin.
  2. I can refactor this code later on today. However, I’m concerned about safeEval because of the inefficiency of building the function each time, when the same function is used multiple times. How would you suggest breaking up the call to Function from the assignments?
KargJonas commented 5 years ago

Yeah - 50kb seems like a lot of overhead.. We'll find a better solution :) It's clear that we need a separate function for evaluating code, the nice API of safeEval() was just too tempting not to implement :sweat_smile: . Maybe something like this:

const fn = constructFunction(js, ['arg1', 'arg2', 'arg3']); fn.run({arg1: 42, arg2: 3.1415, arg3: 0}, element);

KargJonas commented 5 years ago

I added bundling in #29 which should we merge first?

KargJonas commented 5 years ago

34