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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

When generating a new app with --embroider use all optimisation flags #10370

Merged
merged 3 commits into from Oct 7, 2023

Conversation

mansona
Copy link
Member

@mansona mansona commented Sep 8, 2023

Firstly I don't know if this sort of change needs an RFC 馃 let me know if you think it does 馃憤 Edit: we agreed that this wasn't RFC material 馃憤

My reasoning for this change is that we are at the point now that when someone generates a new app with --embroider it's reasonable for them to have all the optimisations turned on by default and for them to turn them off as needs be. This will prevent people from staring with a new Embroider app and adding code that can't be optimised accidentally rather than actually consciously opting in/out of certain behaviours

@mansona mansona changed the title When generating a new app with --embroider use all flags When generating a new app with --embroider use all optimisation flags Sep 8, 2023
@NullVoxPopuli
Copy link
Contributor

I'm a big fan. I would be pretty annoyed if this were an additional RFC.
We'll still need an RFC for making embroider default, but we can cross that when we get there

@mansona
Copy link
Member Author

mansona commented Sep 8, 2023

@NullVoxPopuli I don't think it would be annoying for this to be an RFC, I'm happy to go through that process if we think it warrants it 馃憤 I actually started writing this as an RFC first but then thought I would chance it as a PR first 馃槀

@kategengler
Copy link
Member

I don't think this needs an RFC. It is for new projects and as you say we would want them to start optimized. Anybody upgrading with the --embroider flag would see the change and presumably understand whether they can turn on those flags.

staticHelpers: true,
staticModifiers: true,
staticComponents: true,
staticEmberSource: true,
skipBabel: [
Copy link
Contributor

Choose a reason for hiding this comment

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

in my ember-cli-build, I also have these:

    // ember-inspector does not work with this flag
    // staticEmberSource: true,
    splitControllers: true,
    splitRouteClasses: true,
    packagerOptions: {
      webpackConfig: {
        // highest fidelity sourcemaps at the cost of speed
        devtool: 'source-map'
      }
    }

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, if ember-inspector does not work with that flag, I am less sure about recommending it be on by default, even with the --embroider flag

Copy link
Contributor

Choose a reason for hiding this comment

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

well, its a very new flag, and still opt in.

all the other options are about preventing userland code from regressing to be non-vite compatible (fully static)

staticEmberSource is kind of along those same lines (e.g.: "make sure you don't use the private require APIs!"),

but small progress is still progress, and I think this PR is good progress. Because there is no worse feeling than "trying the new thing", only to find out that "you weren't good enough", because the static flags weren't enabled by default.

in the afternoon RFC meeting today, we talked about a strategy for fixing the inspector in the long term for future ember-source changes (similar to what we do for supporting ember-data), and I really like that approach.

@mansona
Copy link
Member Author

mansona commented Oct 4, 2023

I converted this PR to draft because I have a WIP commit on the end to see if it fixes issues with embroider on CI. If it does I will go through and do a release of embroider and update this PR so it's ready to merge 馃憤

@mansona mansona force-pushed the optimized-embroider-default branch 2 times, most recently from 3af639e to db7bbb5 Compare October 5, 2023 06:49
@mansona mansona force-pushed the optimized-embroider-default branch from db7bbb5 to 1052c73 Compare October 6, 2023 20:42
@mansona mansona marked this pull request as ready for review October 6, 2023 20:46
@mansona mansona added this pull request to the merge queue Oct 7, 2023
Merged via the queue into master with commit d812a04 Oct 7, 2023
11 checks passed
@mansona mansona deleted the optimized-embroider-default branch October 7, 2023 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants