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

Introduce import.meta.glob #939

Merged
merged 5 commits into from
Nov 10, 2023
Merged

Introduce import.meta.glob #939

merged 5 commits into from
Nov 10, 2023

Conversation

ef4
Copy link
Contributor

@ef4 ef4 commented Jul 26, 2023

@github-actions github-actions bot added the S-Proposed In the Proposed Stage label Jul 26, 2023
@ef4 ef4 mentioned this pull request Jul 27, 2023
```js
import { setup } from 'ember-cli-mirage';

setup(import.meta.glob('./mirage/**/*.js'))
Copy link
Contributor

Choose a reason for hiding this comment

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

this would immensely help the migration path to ember-mirage

cc @bgantzler / @cah-brian-gantzler

Choose a reason for hiding this comment

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

Not only the migration path, but if its an approved way to discover files, could put the auto import of all the mirage files back in instead of you having to import them individually and create the hash


### Globally powerful import.meta.glob

We could attempt to allow more globally-powerful `import.meta.glob`. For example, it might be possible to make patterns starting with `/` always search the current application, even when an addon is asking. This would give addon authors more of the power they're used to having, but I think it's a much riskier feature to enable across the ecosystem. I'm not convinced we could make it work at reasonable cost in even all current build systems, never mind future ones. As written, this RFC has low-risk of causing compatibility problems in the future since the feature is not allowed in addon publication format. This makes it much easier to evolve the feature over time without breaking the universe.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm in favor of not supporting this behavior


### Additional Vite feature space

Features from Vite's implementation that I didn't incorporate because I don't want to sign us up to reimplement them in every build system:
Copy link
Contributor

Choose a reason for hiding this comment

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

Features from Vite's implementation that I didn't incorporate because I don't want to sign us up to reimplement them in every build system:

- absolute imports, starting with `/`. This is not a well-defined concept in today's Ember apps because they do not have a single directory representing the app's web root.
- eager mode, so that you gain synchronous access to the matching modules
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I worked this out between the two RFCs, but wanted to keep my thought process here in case others have the same/similar thoughts


I've used eager in my spikes of wiring up Qunit in Vite.

since we're using native modules though, and native modules support top-level await, we could effectively re-implement the behavior, if anyone needs(?) this way:

import * as QUnit from 'qunit';
import { setup } from 'qunit-dom';

setup(QUnit.assert);

const moduleRequests = import.meta.glob('./**/*test.{js,ts}');

// loads all the test files, which are then registered with QUnit
await Promise.all(Object.values(moduleRequests).map(loader => loader()));

Though, this requires that our distribution format is ESM, yeah? like, we couldn't keep shipping AMD if we wanted to do something like this.
(

I think that's the point of the other RFC tho?: #938

in particular,

  • we cannot implement other standard ECMA features like top-level await until these incompatible APIs are gone (consider: how is require() supposed to deal with discovering a transitive dependency that contains top-level await?)

)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't want to create a hard dependency between this change and top-level await, so I don't think we should say that's the upgrade path. We want to implement import.meta.glob in classic builds too, where we may never support top-level await.

But we don't need top-level await to achieve a compatible way to wire up tests. Both because (1) Embroider already synthesizes a test entrypoint that works in vite, and (2) even if you wanted to redo that to use import.meta.glob, it's up to us when to start the qunit tests.

QUnit.config.autostart = false;
Promise.all(Object.values(moduleRequests).map(loader => loader())).then(() => QUnit.start());

I do think your question points toward further simplifications of our programming model though. It would be possible to codify the "what is an ember app and how do you boot it" in terms of import.meta.glob. Today the conventions are implicit and Embroider and classic both try to implement them by having opinions about what files to stitch together into the entrypoint of an application. But we could retcon that in terms of import.meta.glob and make it visible to users.

This would be for a v2 app RFC and/or Polaris Routing RFC. But you can imagine how it would work:

const MyRouter = Router.map(import.meta.glob("./routes/**/*.ts", { ... })

The examples @wycats showed at EmberConf were similar but they said Router.map(import.meta, { ... }), and I've confirmed that's not really going to work in existing build tools like Vite. They have the static requirement on import.meta.glob that I've encoded in this RFC. Passing the output of import.meta.glob though would work, and would make it clear which "entry points" you're telling the app to work with.

`import.meta.glob` has this signature:

```ts
(pattern: string): Record<string, Promise<unknown>>
Copy link

Choose a reason for hiding this comment

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

seems Record value should be a function

(pattern: string): Record<string, () => Promise<unknown>>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is correct, thanks, that's just a bug in my text.

@ef4 ef4 added S-Exploring In the Exploring RFC Stage and removed S-Proposed In the Proposed Stage labels Aug 4, 2023
@ef4
Copy link
Contributor Author

ef4 commented Aug 4, 2023

Moved to exploring in RFC review meeting.

@ef4
Copy link
Contributor Author

ef4 commented Aug 18, 2023

Status here: I have some minor fixes to do and I want to propose adding eager: true before saying this is ready to consider for advancement.

@ef4
Copy link
Contributor Author

ef4 commented Oct 27, 2023

I added eager mode and this is now ready for a second look and I hope we can move it to accepted on the next RFC review iteration.

@achambers
Copy link
Contributor

achambers commented Nov 3, 2023

We're fine with this one - from the morning RFC Review ;)

@NullVoxPopuli
Copy link
Contributor

Plugin for adding support to import.meta.glob to non-embroider apps:

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.

5 participants