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

Project Utils: Build App SSR source files shouldn't be hard coded #1218

Closed
latotty opened this issue Sep 9, 2020 · 3 comments · Fixed by #1220
Closed

Project Utils: Build App SSR source files shouldn't be hard coded #1218

latotty opened this issue Sep 9, 2020 · 3 comments · Fixed by #1220

Comments

@latotty
Copy link
Contributor

latotty commented Sep 9, 2020

This is:

  • Feature request

Specifications

  • version: 4.10.0

Expected Behavior

When I set up SSR for site, I would like to define where the index.html and the appComponent factory is located.

Actual Behavior

Currently it is hard coded to "./build/index.html" and "./src/App".

Detailed Description

In my codebase I structured the code differently, I separated the createApp from the App file. And I also use kebab case.
And I would like to use the provided SSR bundler instead of my custom one.

Possible Solution

In my codebase, I 'forked' the current SSR bundler into a package, and added an option to provide the file paths optionally.
Example:

      // Build SSR bundle using app build output
      await buildAppSSR(
        {
          ...options,
          app: __dirname,
          ssrOptions: {
            indexHtmlPath: path.resolve(__dirname, 'build', 'index.html'),
            createAppPath: path.resolve(__dirname, 'src', 'create-app'),
          },
        },
        context,
      );

I replaced the:

const indexHtml = path.resolve(app, "build", "index.html").replace(/\\/g, "\\\\");
const appComponent = path.resolve(app, "src", "App").replace(/\\/g, "\\\\");

With:

  const indexHtml = (ssrOptions.indexHtmlPath || path.resolve(app, 'build', 'index.html')).replace(/\\/g, '\\\\');
  const appComponent = (ssrOptions.createAppPath || path.resolve(app, 'src', 'App')).replace(/\\/g, '\\\\');

And added ssrOptions to the args:

module.exports.buildAppSSR = async ({ app = null, output, ssrOptions = {}, ...options }, context) => {

I think this could be handled more elegantly 😉

I prefer named exports, but I think for this use-case (bundling) default exporting the factory function is more convenient.

@Pavel910
Copy link
Collaborator

Pavel910 commented Sep 9, 2020

@latotty you could approach it in more than 1 way, that's why we have webiny.config.js where you can use your own tools however you like it. That's the whole point of Webiny - give structure, but allow full customization.

The built-in SSR setup is very basic, but works for 95% of cases. If you're willing to send another PR with your improvements, that would be great! 👍

@latotty
Copy link
Contributor Author

latotty commented Sep 9, 2020

@Pavel910 Sure thing! I saw the buildAppSSRFromSource where I can provide my own bundler, but I really like your solution, and I think it would save time for most of the users if they could use the 'default' one, just with a bit of customisations 😉

I used ssrOptions in my solution, do you like it, or should I just spread the options (indexHtmlPath, createAppPath) like you did with the app option?

@Pavel910
Copy link
Collaborator

Pavel910 commented Sep 9, 2020

@latotty sure, let's expand the options. I think we don't need the extra ssrOptions key, let's just put the options in the same level with app, since that function only handles SSR:

await buildAppSSR(
  {
    ...options,
    app: __dirname,
    indexHtmlPath: path.resolve(__dirname, "build", "index.html"),
    createAppPath: path.resolve(__dirname, "src", "create-app")
  },
  context
);

Feel free to add more options if you find them useful :)

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

Successfully merging a pull request may close this issue.

2 participants