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

Hide core folder away from Bedrock users #320

Open
Wolfr opened this issue Jan 18, 2020 · 9 comments
Open

Hide core folder away from Bedrock users #320

Wolfr opened this issue Jan 18, 2020 · 9 comments
Labels

Comments

@Wolfr
Copy link
Contributor

Wolfr commented Jan 18, 2020

I wanted to document the idea around hiding the core folder publicly. This was an idea from @thomastuts .

  • Hiding the core folder for the end user
  • The user starts a new Bedrock project by cloning a Github template, the template has bedrock@next as a devDependency
  • It comes with the right npm scripts to run Bedrock in watch mode, to build it etc.

Why?

  • To deprecate bedrock-cli as a separate project
  • To make sure people don't mess around with the core folder inadvertently

Result

  • Projects are cleaner
  • Less bugs by people changing things in core (this is not an actual problem)

We decided to not do anything in this vein right now, but to park the idea for later.

@Wolfr Wolfr added the idea label Jan 21, 2020
@Wolfr
Copy link
Contributor Author

Wolfr commented Nov 5, 2020

I still like this idea.

@Wolfr
Copy link
Contributor Author

Wolfr commented Mar 9, 2021

I don't know if I really like this idea anymore, it makes it harder to change something in core which can then be ported to Bedrock main.

@Wolfr Wolfr added the onhold label Mar 9, 2021
@thusc
Copy link
Contributor

thusc commented Jul 14, 2021

I really like this idea and I believe it can easily be made to work even if you plan to change things in core and contribute them. (In my opinion, this is more than just hiding core/, but also hiding the rest of Bedrock, and achieve a better separation of concerns. I can easily imagine users of Bedrock having changes in one project and then having a hard time re-using them in another project.)

Running a custom core

Actually, in https://github.com/smartcoop/design I've only committed the content/ directory that comes with Bedrock, but not the core/ or any other files from the Bedrock repository. As I'm playing around to see if indeed I can have Bedrock installed as a regular tool, I have run npm install path/to/bedrock from the design/ directory and as a first approximation, this seems it works.

I have actually installed a branch of mine, where a CLI tool is exposed, called bedrock to try out this idea.

What's interesting is this:

$ pwd
/home/thu/work/smart/design

$ ls -l node_modules/
total 0
lrwxrwxrwx 1 thu users 13 Jul 14 11:04 bedrock -> ../../bedrock

$ ls -l node_modules/.bin/
total 0
lrwxrwxrwx 1 thu users 27 Jul 14 11:04 bedrock -> ../bedrock/core/cli/bedrock

Yes, those are symlinks. This means that I can change Bedrock's source in its own directory, and call it from the design/ directory, without having to re-install it with npm install. So I think this solves the problem of being able to run a custom core and contribute changes (with proper Git history, branches, ability to try it on different projects, ...).

Things to do

To make Bedrock a standalone tool that doesn't require to have a copy of its source in a given project, I see a few things to do:

  • Expose a CLI. I've tried that in the cli branch at https://github.com/thusc/bedrock/tree/cli. It's a bit crude as it simply wraps the Gulp tasks but seems to work.
  • Load the configuration from the project's root directory. I think I can create a PR this evening to show the change, which shouldn't have an impact on existing installs.
  • Load the Pug templates with a better separation between the one found in core, and the one found in content/.

The last point needs a bit more thinking. Pug only provides a single basedir configuration, so currently it is possible to include things either from an absolute path (relative to basedir), or a relative path (relative to the current file). This means we could decide to have basedir be the core/ directory, allowing user templates to load core Pug templates by using absolute path, and to load their own templates by a relative path. This scheme seems to fall apart because the core templates also include user templates. For instance:

core/templates/layouts/sample.pug:include ../../../content/templates/_mixins/all

To solve that problem, I have thought about defining some variable but Pug doesn't support variable interpolation with the include statement, see pugjs/pug#569.

Some people requested the ability to have multiple basedirs but it's not planned: pugjs/pug#1844.

But it seems multiple basedirs can be achieved by a plugin, see https://github.com/gryphonmyers/pug-multiple-basedirs-plugin and pugjs/pug#2499.

I will give a shot in that direction.

@Wolfr
Copy link
Contributor Author

Wolfr commented Jul 14, 2021

Wow, cool. It's been a while since we discussed this idea and we kind of left it open.

FYI we have a global upgrade CLI that can be found at https://github.com/usebedrock/bedrock-cli . It also once contained some functionality to start a new Bedrock version based on a certain branch (we called that Bedrock bases.

I just wrote a blog post about manual updates (slightly awkward but this is the way it is now) - https://bedrockapp.org/2021/07/14/bedrock-1-39-pre-release-warning/ . If we can prevent forcing people to update files manually by being smarter all-around it is worth discussing.

thusc added a commit to thusc/bedrock that referenced this issue Jul 14, 2021
Instead of loading the configuration using a relative path from
core/, load it from the current working directory.

This means that a Bedrock command should be called necessarily from a
project root directory (where the config is supposed to live), but I
guess this was already the case when using commands defined in the
Gulpfile or in the package.json file.

In other words, this change shouldn't have an impact on existing
installs, but paves the road to allow calling Bedrock from projects that
don't have the full source code of Bedrock within their own directory,
and provide their own configuration files.

See usebedrock#320 for details.
@thusc
Copy link
Contributor

thusc commented Jul 14, 2021

So I think I manage to solve this problem. Now I have to choose some naming scheme to properly separate user templates and Bedrock's.

The idea is that we need an absolute path prefix to refer to Bedrock's files. I'm proposing, from a user perspective, that /bedrock/... refers to everything that lives in core/templates/. And that everything else refer to user content, which can live everywhere they want in their project because this will be configurable. I think this means that they have to be careful to not have content under a bedrock/ directory (easy), but that they can also have a bedrock/ directory if they want to replace default templates. I guess this is nice.

In practice, this means I'll have to move the Pug file under core/templates/ to something like core/templates/bedrock/. (The discriminating prefix has to live under one of the basedirs, so the bedrock name should exist even if it seems redundant.)

What do you think ?

This means that user content such as this line in content/templates/_components/aov-icons/02-icon-font.pug

include ../../../../core/templates/mixins/icon-overview

will have to be changed to

include /bedrock/mixins/icon-overview

This is not a very fun migration to ask to users, but the result is much clearer.

@Wolfr
Copy link
Contributor Author

Wolfr commented Jul 15, 2021

To be honest, there are not that many Bedrock users out there except Mono. We've been maintaining this project as an open source project with care and attention to documentation & outside communication; but there is no big usage (yet!).

I don't mind some manual migrations. The line mentioned is an exception that doesn't occur that often. I think people should ue the system mixins and not reference template from core in content.

If that's still in Bedrock's source that's a bit of a leftover from how we used to do it.

Most projects also stay with their Bedrock version that they start with because they are used for prototyping purposes (to be used only until the integration has been completed).

So changing things to the core is definitely OK, as long as we are evolving and they have a good reason.

@Wolfr
Copy link
Contributor Author

Wolfr commented Jul 15, 2021

I had to revert 385497f because it creates a problem with Bedrock's core JS that doesn't correctly import the config anymore after this change.

@thusc
Copy link
Contributor

thusc commented Jul 15, 2021

Oh really sorry. Can you describe how I can reproduce the problem ?

@Wolfr
Copy link
Contributor Author

Wolfr commented Jul 15, 2021

I looked into why config was loaded in those files and there's no good reason anymore, so I brought back your change and got to remove some unneeded code. So problem solved!

@Wolfr Wolfr removed the onhold label Jul 28, 2021
@Wolfr Wolfr mentioned this issue Nov 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants