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.

Sign up for GitHub

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

Jump to bottom

Proposal for dynamic panes #99

Closed
wants to merge 14 commits into from
Closed

Proposal for dynamic panes #99

wants to merge 14 commits into from

Conversation

Vinnl
Copy link
Contributor

@Vinnl Vinnl commented May 27, 2019 •
edited
Loading

(This PR builds on #100, and is thus easier to review if that is merged first.)

This Pull Request contains a new Pane ("scratchpad", similar to pad) whose only goal is to demonstrate the concepts. It is intended as a starting point for a discussion of how best to implement dynamic loading of panes, and how to structure a Pane's code. In other words: feel free to liberally criticise anything in here :)

It proposes that:

  • Pane definitions no longer rely on global variables like window.UI, because a developer can then not reliably trace where a piece of code came from. Instead, e.g. store instances are passed as parameters - which is why this depends on https://github.com/solid/solid-ui/pull/64, and why outline/manager.js was modified.
  • Panes are defined in separate Node packages, even though they can share some common ceremonial code (e.g. Webpack config). This allows them to be distributed independently from the data browser, without us having to reinvent a package manager and all its associated problems.
  • Code to manipulate data in the store (i.e. data.ts) is decoupled from the Pane definition (although it's currently in the same package). This makes it easier for third parties to create apps that manipulate the same data in the same way. It could also allow for e.g. a dashboard-like replacement for the current HTML home page of the data browser, which can re-use that code to e.g. initialise new resources that can be read by that pane.
  • Not relying on the label method to determine whether a pane should be shown, to lower the barrier to entry for new developers.
  • The view is still somewhat cumbersome, but at least it's tested without firing up a fickle browser (using jsdom).
  • A wrapper connecting the proposed pane definitions to the data browser's current methods. This enables the new pane definitions to avoid relying on global data directly, and thereby potentially be included more easily in e.g. a new data browser, a single-pane shell (i.e. pane-to-app converter), or a storybook.

Open challenges:

  • The pod server should expose to the data browser which panes are installed and hence available to use - at least as a first step; a later step could be discussing loading panes from arbitrary locations, as that comes with its own set of challenges.
  • How much freedom do we want to give to panes? In the current implementation, a pane receives a DOM element that it can fill with whatever they want. Pane authors could decide to use React, for example, which means that if two panes get loaded that both use React, they both instantiate their own render trees. It also means that if, e.g. a user switches away from a pane and back again, that pane cannot efficiently preserve and restore its state.

An overview of what's what in this PR, to hopefully make it easier to review:

  • Stricter ESLint and test coverage setup (for TypeScript files). Moved to Refinement of test setup #100.
  • packages/panes: this is where separate Node packages (i.e. with their own package.json) for panes could live.
  • packages/initialise.js (and other scripts it references): since npm run watch only compiles and starts solid-panes, this script does the same for all panes in separate packages inside packages/panes.
  • view.ts and data.ts: view and data access for a pane (scratchpad, in this case). And their test files.
  • paneWrapper.ts`: interface between the old pane definition and the new one.
  • packages/panes/webpack-config-panes: the only non-pane inside packages/panes. This exports a method that can be used to initialise the Webpack config for a pane in a separate package. Currently only used by the scratchpad, but allows different panes to re-use the configuration if needed.
  • pad/padPane.ts: a rewrite of the regular pad Pane from Javascript to TypeScript, no functional changes. Mostly done to be able to compare scratchpad.

Apologies for the enormous code drop, but given that it's quite a task to move to dynamic panes, I'm afraid there's just a lot to discuss. As said, nothing here is set in stone, as far as I care.

Fixes #72.

@Vinnl Vinnl requested a review from megoth May 27, 2019 14:14
Vinnl added 14 commits May 28, 2019 10:42
@Vinnl
Allow unused vars if prefixed with _
750e9b1
@Vinnl
Fail test if linting fails
e4528f8
@Vinnl
Make unit test sessions interactive
28a2601
@Vinnl
Set test coverage to 100%
5b675fd 
Note that not all lines actually need to be covered; however, they
should explicitly be marked as ignored for coverage, preferably
with a comment explaining why they need not be tested.

This only applies to TypeScript files, to ease a gradual
transition.
@Vinnl
Fork rdflib typings to fix gaps in the definitions
80bec92
@Vinnl
Convert the Pad Pane to TypeScript
86d3e3a
@Vinnl
Experimental new pane setup
f2d0894 
This commit introduces a new Pad pane that can read Pads created by
the old one. It is intended as a testbed for experimenting with a
new setup for Panes in which their concerns are more clearly
separated. Specifically, it aims to achieve the following goals:

- Extract data access into a separate module. This potentially
  allows these modules to be re-used in other apps, to ensure
  data compatibility.
- Separate DOM manipulation from data manipulation. This makes it
  easier to update the view without risk of breaking something
  else.
- Add a wrapper to intermediate between the old and the new API.
  Hopefully we can let go of the old API in time, but that first
  requires more clarity on what needs to be preserved.
- Separate creation of new items from viewing them. Since the
  data model can be shared between different apps/panes, no single
  pane should be responsible for creating new instances, as that
  would be redundant with the equivalent methods in different
  panes.
- Separate item viewing from item discovery. Panes are responsible
  for viewing specific pieces of data (in this case: a notepad).
  This is useful for e.g. when a user is linked to a resource
  directly, allowing them to browse a specific piece of data.
  Discovering what data is available on one's Pod in general is a
  separate task - one could imagine e.g. a listing of simply all
  notepads in a Pod, sorted by date. Clicking one of those would
  bring you to the Scratchpad Pane.

This API is not final.

Note that it makes use of async/await, and therefore imports
@babel/polyfill. Because mashlib overwrites `require`, we cannot
yet include it globally, so the import has to be added to every
file using ES modules and async/await.
@Vinnl
Add an edit mode for the Scratchpad
f7499c5 
This is to make sure use cases that require authentication work.
@Vinnl
Refactor view params to object
973d05a 
This makes it easier to later add additional params, which I expect
to happen.
@Vinnl
Add tests for the Scratchpad view
b6dce1b
@Vinnl
Measure test coverage for TypeScript files
8bfb8ce 
This commit also makes sure that the threshold is achieved, either
by adding tests or ignoring them for coverage (with the reason
explained).
@Vinnl
Show a Scratchpad's author
d6d756b 
Added primarily to test navigating between different resources
without a full page refresh.
@Vinnl
Extract the Scratchpad pane into its own package
c482fc3
@Vinnl
Dynamically detect package names
fbe4bcd 
Note that getPanes() can only be used server-side. To actually load
them, the server should generate client-side scripts that actually
include the pane, preferably without already including the pane's
code in the initial payload.
@Vinnl
Copy link
Contributor Author

Vinnl commented May 28, 2019 •
edited
Loading

I just extracted a few unrelated commits out into #100, so it can more easily be reviewed/merged in pieces. Apologies if you already pulled this branch locally. If so, and you did not make changes yet locally, please run:

git checkout revamp; git branch --delete 72-scratchpad; git fetch; git checkout 72-scratchpad

Vinnl
Vinnl commented May 28, 2019
View reviewed changes
types.ts

// Note: there might be a more appropriate project to hold this definition:
export interface PaneDefinition {
icon: string;
name: string;
label: (subject: Node) => string | null;
render: (subject: Node, dom: HTMLDocument, options?: unknown) => HTMLElement;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First feedback by @megoth: move store to be part of options in render and mintNew. (Not sure if there's a better solution for label?)

Reference: https://github.com/solid/solid-ui/pull/64#pullrequestreview-242711412

@megoth
Copy link
Contributor

megoth commented May 28, 2019

For others reading this - this is a proposal connected to #103

@megoth
Copy link
Contributor

megoth commented May 28, 2019

As noted in this comment we'll postpone this work until we have more work done on other parts of the data browser.

I do think there are some code in this PR that we'll merge in as separate PRs, but for now I'll close this PR.

@megoth megoth closed this May 28, 2019
@timea-solid timea-solid deleted the 72-scratchpad branch February 28, 2022 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@megoth megoth Awaiting requested review from megoth

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Vinnl @megoth