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

Asset Import Spec #763

Merged
merged 7 commits into from
Oct 27, 2023
Merged

Asset Import Spec #763

merged 7 commits into from
Oct 27, 2023

Conversation

ef4
Copy link
Contributor

@ef4 ef4 commented Aug 13, 2021

@NullVoxPopuli
Copy link
Contributor

Rendered (post file rename)

@chancancode
Copy link
Member

How would importing assets from CSS work? eg images and fonts?

@mydea
Copy link
Contributor

mydea commented Aug 16, 2021

Do I understand correctly, that if I'd want dynamic filenames I'd have to continue using /public? E.g. something like this:

export default class FlagComponent  extends Component {
  get url() {
    return `/assets/flags/${this.args.country}.png`;
  }
}

I know that also does not "just work" with the current broccoli-asset-rev fingerprinting, but might be interesting to mention how dynamic asset names would/should work under the new format (which I am generally def. in favor of!). Depending on the use case, I guess either an exhaustive map (if you don't have too many options) would work fine, but for larger lists of options (e.g. country flags) it might be a bit "overkill":

import flagDe from 'my-app/flags/de.png';
import flagAt from 'my-app/flags/at.png';
import flagUk from 'my-app/flags/uk.png';

const flagMap = {
  de: flagDe,
  at: flagAt,
  uk: flagUk
};

export default class FlagComponent extends Component {
  get url() {
    return flagMap[this.args.country];
  }
}

@ef4
Copy link
Contributor Author

ef4 commented Aug 18, 2021

How would importing assets from CSS work? eg images and fonts?

I should be more explicit about it in the text, but what I meant by:

From Type To Type Example Syntax Semantics
CSS CSS @import("thing/style.css") W3C plus NodeJS resolving rules
CSS Other url("thing/icon.png"); W3C plus NodeJS resolving rules

is that we promise that those syntaxes will obey their original W3C semantics, with the addition of the ability to do node-style package resolution (so that the imported paths have a consistent meaning between here and in JS).

Like with JS, how we actually implement the semantics is left unspecified. @import for example is totally allowed to be inlined, or not. url() could end up containing a data URI, etc.

Providing the option to fine-tune the implementation is left up to each specific bundler. In a classic build, that would be ember-auto-import and its webpack.

@wagenet
Copy link
Member

wagenet commented Jul 23, 2022

What's the status on this? Should we get more formal review?

@chriskrycho chriskrycho added the S-Exploring In the Exploring RFC Stage label Dec 1, 2022
@wagenet wagenet marked this pull request as draft December 2, 2022 20:03
@achambers achambers mentioned this pull request Sep 1, 2023
@achambers
Copy link
Contributor

Context: @kategengler has some updates to the RFC here: #973

@ef4 ef4 marked this pull request as ready for review October 13, 2023 18:29
@ef4
Copy link
Contributor Author

ef4 commented Oct 13, 2023

I have merged updates from @kategengler and taken this out of draft, please take a look again as we think it's ready to advance.

@simonihmig
Copy link
Contributor

On the implementation side, how does this RFC relate to embroider-build/ember-auto-import#565? My understanding would be, that a good part of this RFC's implementation is to get that eai feature to land, the other part is webpack/webpack#16693 and some additional default webpack config to set up the asset loader, is that fair to say?

On the eai feature, as it is currently envisaged, it would require users to opt-in by specifying a pattern for all the stuff that eai should handle, that would have traditionally been part of the classic build. But it seems according to the RFC importing anything (that's not a module or CSS I guess?) is supposed to "just work", correct?

@ef4
Copy link
Contributor Author

ef4 commented Oct 27, 2023

@simonihmig this RFC no longer proposes using import statements for assets. It proposes import.meta.resolve().

The implementation of that feature in classic apps might very well be handled by ember-auto-import, but it would not require any config if we decide to land this RFC and make it standard.

@ef4 ef4 merged commit f024c1e into master Oct 27, 2023
15 checks passed
@delete-merged-branch delete-merged-branch bot deleted the asset-importing-spec branch October 27, 2023 18:16
@simonihmig
Copy link
Contributor

@ef4 I understand the recent changes in this RFC. But I still feel there is a bit of overlap between this RFC and the proposed eai feature, that could lead to some inconsistencies...

For example, under this RFC you would be able to import.meta.resolve('./style.css') if you wanted to, right? If you don't want just the URL, but the side-effect of applying the CSS, you could do import './style.css', but only for v2 addons and Embroider apps. For v1 addons or classic apps, that would be possible with that eai feature, but as it stands only with some additional configuration.

For a more complete map of which kind of asset handling is possible:

Expression v1 addon v2 addon classic app Embroider app
import.meta.resolve('./logo.png') 🟢 🟢 🟢 🟢
import.meta.resolve('./style.css') 🟢 🟢 🟢 🟢
import logo from './logo.png' 1 🟠🟡 🟡 🟠🟡 🟡
import './style.css' 🟠 🟢 🟠 🟢
import.meta.resolve('./data.yml') 🟢 🟢 🟢 🟢
import parsedData from './data.yml' 🟠🟡 🟡 🟠🟡 🟡

🟢 works out of the box, according to this RFC / RFC507 (v2 spec)
🟡 needs custom webpack loader configuration
🟠 needs embroider-build/ember-auto-import#565 and custom config

The 🟡 part of needing custom config for importing unexpected file types is expected, and not my point here. But my point is about the 🟠 part of being able to import your own assets, where v1 addons/classic apps need that eai feature with a custom config (which btw is not backed by an RFC), while this "just works" for v2 addons/Embroider apps.

This RFC makes import.meta.resolve() work symmetrically across all types of addons/apps. Shouldn't we strive for similar symmetry for the other cases?

Implementation-wise, I would think this shouldn't be a big problem when we have that eai feature, given that it was never possible to import anything but JavaScript for v1 addons and classic apps, so requiring a config is not really need. We could just enable eai to handle any (local, relative) import without any explicit config, when that import refers to anything that is not JavaScript, couldn't we? Which would make the table above symmetric again.

Does that make sense? And if so, should this be a short follow-up RFC?

Footnotes

  1. this example is superseded by this RFC, but still possible to set up

@balinterdi
Copy link

Rendered (for some reason, the URL has changed again).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Final Comment Period S-Exploring In the Exploring RFC Stage
Projects
None yet
Development

Successfully merging this pull request may close these issues.