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

ember-svg-jar hbs strategy broken #746

Open
betocantu93 opened this issue Mar 27, 2021 · 9 comments
Open

ember-svg-jar hbs strategy broken #746

betocantu93 opened this issue Mar 27, 2021 · 9 comments

Comments

@betocantu93
Copy link
Contributor

betocantu93 commented Mar 27, 2021

Hello 👋🏼 recently the ember-svg-jar new hbs strategy got merged 🎉, native dynamic imports for template only components generated from svgs at build time, thanks @ef4 for the help back then in contributors workshop, here's the strategy: evoactivity/ember-svg-jar#186! embroider is on the ember-try matrix and it passed before merging...

Until I added a follow up PR evoactivity/ember-svg-jar#189 with an example on the dummy/app/templates/application.hbs and a found out that its actually not embroiderSafe()... I started by changing the code to make it statically analyzable i.e. not using {{component}} or string invokation...

The dynamic import part:
https://github.com/betocantu93/ember-svg-jar/blob/15a250980af94e4c1d20b8240de56ccbd8748d58/addon/components/svg/index.js#L33

The ensureComponent part:
https://github.com/betocantu93/ember-svg-jar/blob/15a250980af94e4c1d20b8240de56ccbd8748d58/addon/components/svg/index.js#L51

The template usage part:
https://github.com/betocantu93/ember-svg-jar/blob/15a250980af94e4c1d20b8240de56ccbd8748d58/addon/components/svg/index.hbs#L2

But it seems there are a few roadblocks for this to work with embroider.

Reading through the spec, I "know" or noticed a few things, please correct me if wrong:

  1. Support for treeForAddon and treeForPublic is dropped, since this is v1 addon I can still see the generated components in /dist/ember-svg-jar/components/** with a subtle difference, the templates are not compiled 🤔 <-- not sure if this is actually a compat bug
  2. Dynamic import just don't work, because we have to dynamically build the asset path at runtime cuz of the dynamic nature of these components and accounting for fingerprinting and such.
  3. A little jump in to the future, packages must declare their public-assets statically for addon metadata, this is something this hbs strategy would need to somehow build a la carte(?), but overall I understand v2 addon just don't generate stuff at build time anymore.

If I understood the overall spec correctly, the hbs strategy would actually need to be refactored and exposed as an API or something to the host app for it to manually generate them i.e generate and add them to /public manually on ember-cli-build.js, but even that is feasible (why not), the dynamic import, dynamic asset path constraint, still blocks this.

There's no rush for anything, but I would love to get some guidance on what's need to be done or some help designing the solution and work or co-work on it 😄

Thanks for the amazing work!

P.D I probably miss understood many parts of the spec.

@ef4
Copy link
Contributor

ef4 commented Mar 27, 2021

A key idea with embroider is that apps (and addons) only get to deal with the files as authored, and don't have any say over how those files get delivered. If you want some code to load dynamically on demand, you express that with await import and you leave it up to embroider to decide how that gets delivered.

I think you can make ember-svg-jar work as a v1 addon under embroider by:

  • emitting the built components in treeForApp instead of treeForPublic

  • putting the dynamic loading code also in treeForApp saying something like this:

    await import(`./svgs/${which}`)
  • stopping the embroider compatibility layer from loading these modules eagerly (normally anything under /app that we don't understand gets eagerly loaded and registered with loader.js, purely for compatibility with things that don't understand embroider):

    // ember-cli-build.js
    return require('@embroider/compat').compatBuild(app, Webpack, {
     staticAppPaths: ['svgs']
    });

The tricky bit here is that this will only work under embroider and not under classic builds, and maintaining an embroider-specific implementation of a v1 addon is a kinda weird intermediate state to be in. It's probably better to reimplement as a v2 addon and rely on the (nearly finished!) v2-to-v1 compatibility system to make it work in classic builds too.

There are a few ways ways you'll want to rethink the API to become a v2 addon.

The first is that all this lazy loading stuff doesn't belong in ember-svg-jar at all. It's only needed right now because ember-cli lacks native support for that sort of thing, but that is not the case with embroider. I think ember-svg-jar should handle svgs and not worry at all about lazy loading, if users want to lazy load any component, including one that was built automatically from an SVG, they can just do that.

Another issue is more about upcoming ember changes than about embroider: strict mode templates are coming. Users will be expected to import a component to call it. I think the natural way to do this with ember-svg-jar is to make users import the svg file directly and then call it as a component. For example, using one of the proposed syntaxes from ember-template-imports it could look like:

import Star "./icons/star.svg";

<template>
  <Star />
</template>

The third issue is that v2 addons are not going to be allowed to magically modify the build pipeline. The problem with that is that it locks us forever into one implementation and makes it hard to build apps any other way. Making app authors assemble their configuration is more verbose, but it makes it much easier to evolve into the future, because it makes it practical for some app authors to try a new build tool.

What ember-svg-jar would provide is the necessary configuration that converts svgs into ember components. That is likely to be build-system-specific, but it's better to be up front about that then just de facto lock into one kind of build:

// ember-cli-build.js
const svgJar = require('ember-svg-jar/webpack-config');

return require('@embroider/compat').compatBuild(app, Webpack, {
 packagerOptions: {
   webpackConfig: {
      module: {
        rules: [svgJar()]
      }
   }
 }
});

Then if another packager gets popular with ember apps, you have a natural way to add (for example) ember-svg-jar/snowpack-config. In practice very little glue code would be needed in these utilities, the core functionality (converting svgs to optimized ember components) would all be shared.

@betocantu93
Copy link
Contributor Author

betocantu93 commented Mar 29, 2021

Thanks for the detailed response, and yes it makes total sense to avoid in-between solutions and prob go for the v2 implementation, I think a good example of how the new api could look like is https://github.com/gregberge/svgr gona do some research, probably we could even use it :) thanks again

@betocantu93
Copy link
Contributor Author

betocantu93 commented Mar 30, 2021

Hello! just some follow up (no rush, and thanks in advance for your time), I did some research (new to Webpack TBH).
What I'm trying to do is:

  1. Svg loader: takes an svg, and use template recast to add splattributes and a few conditional markup
  2. Compile the template using @embroider/hbs-loader

For example:

import Component from '@glimmer/component';
import Star from '../../icons/star.svg';

console.log(Star); // this should be a compiled template only component
export default class MyComponent extends Component {}

My current webpack svg loader config:

const { Webpack } = require("@embroider/webpack");
  return require("@embroider/compat").compatBuild(app, Webpack, {
    staticAddonTestSupportTrees: true,
    staticAddonTrees: true,
    staticComponents: true,
    staticHelpers: true,
    packagerOptions: {
      webpackConfig: {
        module: {
          rules: [
            {
              test: /\.svg$/,
              use: [
                {
                  loader: require.resolve("@embroider/hbs-loader"),
                },
                {
                  loader: require.resolve("ember-svg-loader"),
                  options: {},
                },
              ],
            },
          ],
        },
      },
    },
  });

Overall, is this a right approach? or...?

Currently it doesn't work because @embroider/hbs-loader needs a templateCompilerFile which I don't know how to get it 🤔

I also tried a different approach, where the svg-loader would emit javascript

For example:

	let file = `
			import templateOnly from '@ember/component/template-only';
			import { setComponentTemplate } from '@ember/component';
			import { hbs } from 'ember-cli-htmlbars';

			export default setComponentTemplate(hbs\`${code}\`, templateOnly());
	`;

and Webpack:

const { Webpack } = require("@embroider/webpack");
  return require("@embroider/compat").compatBuild(app, Webpack, {
    staticAddonTestSupportTrees: true,
    staticAddonTrees: true,
    staticComponents: true,
    staticHelpers: true,
    packagerOptions: {
      webpackConfig: {
        module: {
          rules: [
            {
              test: /\.svg$/,
              use: [
                {
                  loader: 'babel-loader-8',
                  options: {
                    presets: ['@babel/preset-env']
                  }
                },
                {
                  loader: require.resolve("ember-svg-loader"),
                  options: {},
                },
              ],
            },
          ],
        },
      },
    },
  });

But with this approach nothing gets resolved by webpack i.e @ember/component couldn't be resolved, not sure if the svg-loader needs to have these deps in order to work...

Which approach do you think I should pursuit? also, I think a basic example of how to do this kind of stuff could be super helpful, it can be in one of the test-packages and I can help to write it 😄

One thing I can't get my head around. I'm not sure how it would work for dynamic imports with an expression, does webpack generates all posible chunks automatically when it sees something like:

await import(`./icons/${which}`)

Or how would they exist in runtime if not statically called? Or I would need to create them all "manually" (ala ember-svg-jar) and add them to public and the loader just... resolve them as-is?

@betocantu93
Copy link
Contributor Author

betocantu93 commented Mar 30, 2021

Or how would they exist in runtime if not statically called? Or I would need to create them all "manually" (ala ember-svg-jar) and add them to public and the loader just... resolve them as-is?

so I found that it chunks automatically, calling the svg-loader... tried it in a simple webpack project, so it seems we don't need to eagerly create the components, and let webpack decide if they are needed wether by literal import or with the import('./icons/${iconName}').

So now I just need to know how to compile them I guess, from the svg-loader and then just define an easy way to configure svgo, but that should be simplier

@betocantu93
Copy link
Contributor Author

betocantu93 commented Mar 31, 2021

Based on a screenshot I took from your talk at emberconf (which was awesome, thanks a lot for your contributions!) haha, I manage to provide the template-compiler path to the @embroider/hbs-loader, but... now it complains

Cannot call compile with only the template compiler loaded. Please load ember.debug.js or ember.prod.js prior to calling compile.

return require("@embroider/compat").compatBuild(app, Webpack, {
    staticAddonTestSupportTrees: true,
    staticAddonTrees: true,
    staticComponents: true,
    staticHelpers: true,
    packagerOptions: {
      webpackConfig: {
        module: {
          rules: [
            {
              test: /\.svg$/,
              use: [
                {
                  loader: require.resolve("@embroider/hbs-loader"),
                  options: {
                    templateCompilerFile: join(
                      __dirname,
                      '../../node_modules/ember-source/dist/ember-template-compiler.js'
                    ),
                    variant
                  },
                },
                {
                  loader: require.resolve("@ember-svgr/template-recast"),
                  options: {},
                },
              ],
            },
          ],
        },
      },
    },
  });

@betocantu93
Copy link
Contributor Author

Hello @ef4 hope you doing fine, finally back at this after a few months of backlog.

When you have time and if possible would you mind showing me a little loader Proof of concept that would take strings and compile them to ToC, say for .svg ext? I genuinely tried a bunch of configurations. I can surely take it from there.

Thanks!

@ef4
Copy link
Contributor

ef4 commented Nov 12, 2021

A thing that might help as an example is on this branch I'm currently working on. It includes an even simpler hbs loader for templates:

https://github.com/embroider-build/embroider/blob/template-compilation/packages/hbs-loader/src/index.ts

That uses a string-to-string transform here:

https://github.com/embroider-build/embroider/blob/template-compilation/packages/shared-internals/src/hbs-to-js.ts

Which relies on stacking several webpack loaders together, so that first we go hbs-to-js and then we apply our babel to the JS.

You could do a similar pattern to rewrite svg to JS-with-embedded-templates.

@betocantu93
Copy link
Contributor Author

betocantu93 commented Nov 17, 2021

Hello, thanks for your examples! They really did helped me out to clarify some things regarding the template compiler I was kind of lost there. The good news is that I actually managed to do a PoC, which is great, But with some hacks and so I have some doubts.

When using babel-loader after transforming the svg to js... The packager can't resolve any of the needed imports for the resulting js.

{
  test: /\.svg$/,
  use: [
   {
      loader: 'babel-loader',
   },
  {
     //For demo purposes
      loader: (str) => {
          //Transformed
           const code = "<svg ...attributes> </svg>"

         return [
		`import { hbs } from 'ember-cli-htmlbars';`,
		`import templateOnly from '@ember/component/template-only';`,
		`import { setComponentTemplate } from '@ember/component';`,
		`export default setComponentTemplate(hbs("${jsStringEscape(code)}"), templateOnly());`,
	].join('\n')
      }; 
   }
  ]
}

This supposedly should work, the problem is that none of these imports are resolved by webpack

Module not found: Error: Can't resolve 'ember-cli-htmlbars'
Module not found: Error: Can't resolve '@ember/component/template-only'
Module not found: Error: Can't resolve '@ember/component'

I got it "working" by using the babel-loader-8 and passing the required options cacheDir, variant and appBabelConfigPath which I logged to get them.

{
  test: /\.svg$/,
  use: [
   {
       loader: require.resolve('@embroider/babel-loader-8'),
       options: {
           variant: {
                name: 'dev',
                runtime: 'browser',
                optimizeForProduction: false,
            },
            appBabelConfigPath:        '/private/var/folders/6n/yln_k53d5jl00m00ql5ykl9h0000gn/T/embroider/04e354/packages/testing328/_babel_config_.js',
                    cacheDirectory:
                      '/private/var/folders/6n/yln_k53d5jl00m00ql5ykl9h0000gn/T/embroider/webpack-babel-loader',
         },
   },
   {
     //For demo purposes
      loader: (str) => {
          //Transformed
           const code = "<svg ...attributes> </svg>"

         return [
		`import { hbs } from 'ember-cli-htmlbars';`,
		`import templateOnly from '@ember/component/template-only';`,
		`import { setComponentTemplate } from '@ember/component';`,
		`export default setComponentTemplate(hbs("${jsStringEscape(code)}"), templateOnly());`,
	].join('\n')
      }; 
   }
  ]
}

So my questions are

  1. In the first example I'm not sure why these modules are not resolved, maybe I'm missing something really clear?
  2. Are we supposed or its recommended to use @embroider/babel-loader-8 always? if so, how can I get all the required options for it?

@betocantu93
Copy link
Contributor Author

Hello @ef4 hope you're okay

Which relies on stacking several webpack loaders together, so that first we go hbs-to-js and then we apply our babel to the JS.

I dug and found that I had to use these plugins in order to resolve the deps:

  • @embroider/core/src/babel-plugin-inline-hbs-deps-node.js
  • babel-plugin-ember-template-compilation
  • @embroider/core/src/babel-plugin-adjust-imports.js

The problem is that these dependencies handling plugins are not exported from @embroider/core and also need some args which I don't have access, is there any other way I can bypass the need of this plugins?

Thanks

betocantu93 added a commit to prysmex/ember-eui that referenced this issue Feb 6, 2022
In order to simplify ember 4.x.x adoption and embroider

Use conventional inline svg strategy.

Until further experiments with webpack loaders and post embroider world

embroider-build/embroider#746
betocantu93 added a commit to prysmex/ember-eui that referenced this issue Feb 18, 2022
In order to simplify ember 4.x.x adoption and embroider

Use conventional inline svg strategy.

Until further experiments with webpack loaders and post embroider world

embroider-build/embroider#746
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

2 participants