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

Add support for keeping public assets and ember-addon.public-assets meta in sync #1368

Merged
merged 4 commits into from
Feb 28, 2023

Conversation

phndiaye
Copy link
Contributor

@phndiaye phndiaye commented Feb 25, 2023

While migrating an addon to the v2 format, I noticed that we need to declare the files from the public directory in the ember-addon.public-assets meta in the package.json, which is easy if there isn't much of them, but can get out of hand quickly (I have a couple addons providing a lot of assets to the parent app)

This adds support for keeping an addon's package.json ember-addon.public-assets meta in sync with the public directory by default.

By passing opts.exclude (glob pattern array), you can still ignore some of the files if needed.

export default {
  output: addon.output(),

  plugins: [
    addon.publicEntrypoints([]),

   // Other plugins declared in the samples

    addon.publicAssets('assets'), // Or addon.publicAssets('assets', { include: ['**/*.svg'], exclude: ['**/*.png'] }) to skip all PNG files and include SVG ones
  ]
};

@phndiaye phndiaye changed the title Add support for keeping the public assets and ember-addon.public-assets meta in sync Add support for keeping public assets and ember-addon.public-assets meta in sync Feb 25, 2023
name: 'public-assets-bundler',
generateBundle() {
let pkg = readJsonSync('package.json');
const filenames = walkSync('public', {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To my knowledge, the public folder is not something that's configurable. Otherwise, this could be part of the options (w/ a default value)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think there is any specific path mandated here, according to https://rfcs.emberjs.com/id/0507-embroider-v2-package-format#assets. Also this is about the v2 addon's internal file layout, and that should be seen as an implementation detail.

So I would suggest to make this configurable. Also, maybe we should also have an include pattern that we can pass to glob of walkSync? So users could say e.g. publicAssets({ path: 'assets', include: '**/*.svg'), to make all svgs in ./assets (instead of ./public).

Copy link
Collaborator

@simonihmig simonihmig left a comment

Choose a reason for hiding this comment

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

I think that's a good feature to have!

Although the public folder is IMHO a bit of a remnant of the old (push-based) world. We might instead recommend people to the pull-based world in the future, when we have a default setup for importing static assets (related: #1323, emberjs/rfcs#763). But as we have that feature currently, we can also make it more ergonomic for sure...

@@ -4,6 +4,7 @@ import { default as appReexports } from './rollup-app-reexports';
import { default as clean } from 'rollup-plugin-delete';
import { default as keepAssets } from './rollup-keep-assets';
import { default as dependencies } from './rollup-addon-dependencies';
import { default as publicReexports } from './rollup-public-reexports';
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick: I wouldn't call it (re)exports, as this is not getting imported. Rather it's more of the classic push models vs. pull. Maybe just name it just similar to the public API for users, like rollup-public-assets?

name: 'public-assets-bundler',
generateBundle() {
let pkg = readJsonSync('package.json');
const filenames = walkSync('public', {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think there is any specific path mandated here, according to https://rfcs.emberjs.com/id/0507-embroider-v2-package-format#assets. Also this is about the v2 addon's internal file layout, and that should be seen as an implementation detail.

So I would suggest to make this configurable. Also, maybe we should also have an include pattern that we can pass to glob of walkSync? So users could say e.g. publicAssets({ path: 'assets', include: '**/*.svg'), to make all svgs in ./assets (instead of ./public).

@ef4
Copy link
Contributor

ef4 commented Feb 27, 2023

This seems good and I agree with Simon's feedback.

The one thing I'll point out is that we hope to not make addons use this for most cases. It's actually pretty problematic if addons are putting things in here that the addon plans to access at runtime, because the connection between the consumption site and the asset is not statically visible. It means that people run into trouble when they deploy assets to a CDN or adjust fingerprinting rules, etc, because the arbitrary runtime code needs to know about those changes.

We want to instead allow code to explicitly ask for an asset's URL. In embroider apps today people often use the native webpack 5 asset handling to import the URL of an asset:

import iconURL as "./icon.png";

And that works well but it's not really a standard and we'd like to come up with something less implementation-specific. I'm hoping the import assertions spec proposal actually moves forward again at TC39, because that would create a web-standards-compliant way to do this.

@phndiaye
Copy link
Contributor Author

phndiaye commented Feb 27, 2023

@simonihmig @ef4 Thanks for the review and comments!

I agree the public folder naming here reminds of the older way so I was wondering if it was worth defining it as a default value in the implementation or if we should "force" users to provide a value for it. This would lead to a signature like:

publicAssets(path: string, opts: { include: string[], exclude: string[] })

@ef4 Indeed this should be avoided as much as possible as it kind of negate the work done to make things statically analysable. But there are cases where I find it really useful, for example when implementing a design system and you need to provide all parents apps some branding assets (or when converting existing apps that are not importing assets in JS yet)

Do you think it can be added and documented somehow but not put in the sample Rollup config as it's not the recommended way?

@ef4
Copy link
Contributor

ef4 commented Feb 27, 2023

Even for shared branding assets I think we'd want people to use "the new way". Meaning, users of your addon can either import the assets directly out of the addon, or the addon can provide components that directly import the assets.

But yeah, since there's no standardized "new way" right now I'm fine with moving forward this PR. It can be a documented option that's not enabled by default.

… passed down instead of forcing it to public
@phndiaye
Copy link
Contributor Author

I see... I still have to shape my mental model around this "new way" but I will keep that in mind as I move more addons to the v2 format.

Updated the PR to reflect the feedbacks, by the way.

@phndiaye
Copy link
Contributor Author

Looks like the CI is failing w/ this issue #1366 :/

@ef4 ef4 merged commit 01575a4 into embroider-build:main Feb 28, 2023
@phndiaye phndiaye deleted the public-tree-copy branch March 1, 2023 18:21
This was referenced May 2, 2023
@ef4 ef4 added the enhancement New feature or request label May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants