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

Implement approach for delivering constants to the new frontend components #2708

Closed
NateWr opened this issue Aug 14, 2017 · 7 comments
Closed
Assignees
Milestone

Comments

@NateWr
Copy link
Contributor

NateWr commented Aug 14, 2017

There are a number of magic numbers in the new submission list panels that need to be replaced with proper constants.

Currently, we can pass constants defined in PHP to the JS frontend through the defined_exposed() function. This works at a global level (ie - constants must either be exposed or not exposed when defined). As more logic is handled in the new frontend handlers, we'll need access to more constants in JS land.

I'd propose passing a _constants object with a handler's config. These can then be picked up by VueRegistry.init and merged with a pkp.constants hash.

@NateWr NateWr added this to the OJS 3.1 milestone Aug 14, 2017
@NateWr NateWr self-assigned this Aug 14, 2017
NateWr added a commit to NateWr/pkp-lib that referenced this issue Aug 14, 2017
NateWr added a commit to NateWr/ojs that referenced this issue Aug 14, 2017
NateWr added a commit to NateWr/omp that referenced this issue Aug 14, 2017
@NateWr
Copy link
Contributor Author

NateWr commented Aug 14, 2017

PRs:
#2710
pkp/ojs#1493
pkp/omp#429

There will be some merge conflicts once other recent PRs get merged so I'll refresh the PRs then.

NateWr added a commit to NateWr/pkp-lib that referenced this issue Aug 21, 2017
NateWr added a commit to NateWr/ojs that referenced this issue Aug 21, 2017
NateWr added a commit to NateWr/pkp-lib that referenced this issue Aug 22, 2017
NateWr added a commit to NateWr/pkp-lib that referenced this issue Aug 22, 2017
NateWr added a commit to NateWr/omp that referenced this issue Aug 22, 2017
NateWr added a commit to NateWr/pkp-lib that referenced this issue Aug 22, 2017
NateWr added a commit to NateWr/pkp-lib that referenced this issue Aug 22, 2017
NateWr added a commit to NateWr/pkp-lib that referenced this issue Aug 22, 2017
NateWr added a commit to NateWr/ojs that referenced this issue Aug 22, 2017
@NateWr
Copy link
Contributor Author

NateWr commented Aug 22, 2017

Tests passed. @asmecher mind code reviewing this approach for passing constants to the frontend?

@asmecher
Copy link
Member

@NateWr, why not just use define_exposed (instead of define) where those constants are originally defined? That would reduce the number of backflips.

@NateWr
Copy link
Contributor Author

NateWr commented Aug 24, 2017

One of Vue.js's architecture choices is for components to be explicit about their dependencies and relationships to other components. That's why you have to name a prop when you're passing it down to a component:

<custom-component
  id="item.id"
  title="item.title"
  i18n="i18n"
/>

And then declare it again in the receiving component:

export default {
  props: ['id', 'title', 'i18n'],
  ....
}

That leads to some unnecessary verbosity. We have to name the i18n prop on just about every component we have.

Their argument -- and I tend to agree -- is that this verbosity makes the code easier to read and easier to maintain. Someone working with one component doesn't need to go looking anywhere else to "know" the component. Their single-file components extend this principle so that everything (markup, logic, style) are all housed together. As projects evolve, it's easier to identify dead code. When a component goes, you don't need to discover what associated code you can get rid of. (I'll be bringing styles into the components soon.)

I'd like to extend this to the PHP handlers for a component on the backend. The getConfig method which each handler uses to initialise a component is deliberately broad so that every handler has a single place where it's data structure is defined. Even though we may call parent::getConfig(), it's still housed in the same method in the parent. There's a single trail to follow and we don't have methods in parent handlers which define configuration without any trace in the child handlers. (The grids do this a lot and I'm still discovering them. Yesterday I found: GridHandler::isDataElementSelected().)

For these same reasons, I think a PHP handler which declares it's required constants will be easier to understand and maintain, even though it will be more verbose. I can think of a few cases where it will be helpful:

  1. After we define_exposed, when will we know a constant is no longer needed on the frontend? If I edit a single component and remove the use of a constant there, I can't confidently switch it from define_exposed to define unless I am familiar with every other component. Also, I would have to know where that constant comes from. By declaring it in the handler config, I can confidently remove it without knowing about other components or the objects and DAOs from which many of the constants come.

  2. define_exposed only works on the initial page requests. But components are often loaded in ajax requests (eg - modals). We can write middleware to deliver exposed constants with every ajax request, but it would get tricky determining when we want to intervene (when loading modal content) and when we don't (an API request). And we'd also be more likely to run into situations where a constant is exposed when the component is used in one place and not when it's used in another (because constants aren't declared globally and would only get exposed if the object or DAO where they are defined gets loaded during the current request).

  3. We would handle the issue above by importing the classes with constants into the getConfig method, so they were loaded every time the handler is initialized. But this would only increase the ambiguity over whether or not that import is still required, because we wouldn't have a record of what constants the handler actually wants from that imported file.

@NateWr NateWr assigned asmecher and unassigned bozana Aug 24, 2017
@asmecher
Copy link
Member

The flip side is that all the explicit statements of what-constant-is-needed-where will need to be maintained, and in my experience it'll lead to a cruft buildup as constants are added but never removed. We can see this aplenty in the locale files, import statements, etc.; if we were able to delegate all imports to an autoloader and do away with import throughout, we'd probably lose a few thousand testy KLOCs nobody would miss. But I take your point about vue.js's philosophy and I'm content to start with your proposal; if it becomes a problem we can rethink it later.

NateWr added a commit to NateWr/pkp-lib that referenced this issue Aug 28, 2017
NateWr added a commit to NateWr/pkp-lib that referenced this issue Aug 28, 2017
NateWr added a commit to NateWr/pkp-lib that referenced this issue Aug 28, 2017
NateWr added a commit to NateWr/ojs that referenced this issue Aug 28, 2017
NateWr added a commit to NateWr/omp that referenced this issue Aug 28, 2017
@NateWr
Copy link
Contributor Author

NateWr commented Aug 28, 2017

I think we're both trying to tackle the same problem, we just maybe see it's source in different places. The locale cruft and the difficulty of maintaining it is one the main reasons I prefer this approach, because it makes explicit the dependencies which are otherwise difficult to uncover. When we remove a constant from a .vue file, we should be able to identify what *Handler.inc.php file to adjust without any worries about side effects.

NateWr added a commit to NateWr/pkp-lib that referenced this issue Aug 28, 2017
NateWr added a commit to NateWr/pkp-lib that referenced this issue Aug 28, 2017
NateWr added a commit to NateWr/pkp-lib that referenced this issue Aug 28, 2017
NateWr added a commit to NateWr/ojs that referenced this issue Aug 28, 2017
NateWr added a commit to pkp/omp that referenced this issue Aug 28, 2017
pkp/pkp-lib#2708 Pass constants with JS handler configuration
NateWr added a commit to pkp/ojs that referenced this issue Aug 28, 2017
pkp/pkp-lib#2708 Pass constants with JS handler configuration
NateWr added a commit that referenced this issue Aug 28, 2017
#2708 Pass constants with JS handler configuration
@NateWr
Copy link
Contributor Author

NateWr commented Aug 28, 2017

Merged. I'm happy to re-evaluate down the line if we feel like it's not addressing our maintenance concerns well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants