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

[WIP] [Fix #862] add emberSourceRealModules flag #866

Closed
wants to merge 6 commits into from

Conversation

stefanpenner
Copy link
Collaborator

@stefanpenner stefanpenner commented Jun 21, 2021

  • changes “real module” mode of ember-source to begin automatically at 4.0.0, rather then 3.27.0-beta.0
  • adds emberSourceRealModules option, to opt into this mode prior to 4.0.0
  • begins fixing [email protected] issues

TODO:

  • sync up with @thoov re: test-matrix stuff
  • debug the expected ember 3.27.x issues
  • possible solution for ^
  • ensure everything works
  • write tests based on discussion with @thoov

@stefanpenner stefanpenner force-pushed the 3-27-fixes branch 2 times, most recently from 6f992e3 to 8d11d3c Compare June 21, 2021 18:48
@stefanpenner stefanpenner added the bug Something isn't working label Jun 21, 2021
packages/compat/src/compat-app.ts Outdated Show resolved Hide resolved
@@ -415,6 +416,17 @@ export class AppBuilder<TreeNames> {
{ absoluteRuntime: __dirname, useESModules: true, regenerator: false },
]);

{
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

needs cleanup

@@ -1480,3 +1492,58 @@ function combinePackageJSON(...layers: object[]) {
}
return mergeWith({}, ...layers, custom);
}

function macroBabelPlugins() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

needs comment

// This flag provideds adventurous ember-source users & addon-authors the
// ability to test & begin preperation prior to this feature becomeing the
// default.
emberSourceRealModules?: boolean | null;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we likely no longer need null here

@stefanpenner
Copy link
Collaborator Author

Test failures so far are just the expected deprecation warning. @rwjblue based on our chat, it would seem like we should allow those deprecations…. Although that does annoy me a little….

@ef4
Copy link
Contributor

ef4 commented Jun 22, 2021

I think if we use modules-api-polyfill it's going to cause deprecation spew on Ember 3.27+?

When code in a true ES module handled by webpack does:

import Component from '@ember/component'`

modules-api-polyfill will rewrite that to Ember.Component, which is deprecated.

We could instead configure webpack's externals so that it knows to defer these imports to runtime and access them via runtime require, which would make us compatible with what the classic build does in this case.

@stefanpenner
Copy link
Collaborator Author

We could instead configure webpack's externals so that it knows to defer these imports to runtime and access them via runtime require, which would make us compatible with what the classic build does in this case.

That approach sounds better. I'll give it whirl

@stefanpenner
Copy link
Collaborator Author

stefanpenner commented Jun 22, 2021

@ef4 It sounds like we may want to essentially add the data from https://github.com/ember-cli/ember-rfc176-data to webpack externals for users 2.27.0-alpha.0..4.0.0-alpha.0 unless the user opts into the emberSourceRealModules.... WDYT?

@ef4
Copy link
Contributor

ef4 commented Jun 22, 2021

@stefanpenner yes, but please try to use it via https://github.com/embroider-build/embroider/blob/7bab34a265e234ab45e8eaf40482f8dbce7aa5cc/packages/shared-internals/src/ember-standard-modules.ts which exists for this purpose.

@ef4
Copy link
Contributor

ef4 commented Jun 22, 2021

@stefanpenner I realized we probably already have enough support for externalizing ember dependencies in adjust-imports-plugin.

You may want to just try not doing anything to externalize the ember modules because this code probably already gets it right:

// auto-upgraded packages can fall back to attmpeting to find dependencies at
// runtime. Native v2 packages can only get this behavior in the
// isExplicitlyExternal case above because they need to explicitly ask for
// externals.
if (pkg.meta['auto-upgraded']) {
return makeExternal(specifier, sourceFile, opts);
}
if (pkg.isV2Ember()) {
// native v2 packages don't automatically externalize *everything* the way
// auto-upgraded packages do, but they still externalize known and approved
// ember virtual packages (like @ember/component)
if (emberVirtualPackages.has(packageName)) {
return makeExternal(specifier, sourceFile, opts);
}
// native v2 packages don't automatically get to use every other addon as a
// peerDep, but they do get the known and approved ember virtual peer deps,
// like @glimmer/component
if (emberVirtualPeerDeps.has(packageName)) {
if (!opts.activeAddons[packageName]) {
throw new Error(
`${pkg.name} is trying to import from ${packageName}, which is supposed to be present in all ember apps but seems to be missing`
);
}
return explicitRelative(dirname(sourceFile.name), specifier.replace(packageName, opts.activeAddons[packageName]));
}
}

@stefanpenner
Copy link
Collaborator Author

stefanpenner commented Jun 22, 2021

You may want to just try not doing anything to externalize the ember modules because this code probably already gets it right:

If memory serves this fails (I dont have the exact failure handy), but I will begin investigation in about 1 -> 1.5h.

@stefanpenner
Copy link
Collaborator Author

I will resume this and now that #881 has landed, I bet some of the issues will be resolved.

@stefanpenner
Copy link
Collaborator Author

rebased (will continue to investigate as time permits)

@stefanpenner
Copy link
Collaborator Author

quick rebase again

@stefanpenner stefanpenner force-pushed the 3-27-fixes branch 2 times, most recently from dc7e5d7 to b8ce0ed Compare September 2, 2021 22:45
@@ -141,6 +141,11 @@ class RewriteManifest extends Plugin {
extraVendorFiles,
};

json['ember-addon'] = {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i don't remember if this was actually important now, i think it isn't and should likely be removed

@stefanpenner stefanpenner force-pushed the 3-27-fixes branch 3 times, most recently from c9b7344 to 3aa84c8 Compare September 3, 2021 04:54
@stefanpenner stefanpenner force-pushed the 3-27-fixes branch 2 times, most recently from 8a3a299 to 2590547 Compare September 7, 2021 17:40
@@ -4,6 +4,7 @@ let rules: PackageRules[] = [
{
package: '@ember-data/store',
addonModules: {
//'-private/index.js': {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

do i need this? I don't even remember.

packages/core/src/app.ts Outdated Show resolved Hide resolved
packages/core/src/app.ts Outdated Show resolved Hide resolved
packages/core/src/babel-plugin-adjust-imports.ts Outdated Show resolved Hide resolved
packages/core/src/babel-plugin-adjust-imports.ts Outdated Show resolved Hide resolved
packages/core/src/babel-plugin-adjust-imports.ts Outdated Show resolved Hide resolved
packages/core/src/babel-plugin-adjust-imports.ts Outdated Show resolved Hide resolved
packages/core/src/babel-plugin-adjust-imports.ts Outdated Show resolved Hide resolved
packages/core/src/babel-plugin-adjust-imports.ts Outdated Show resolved Hide resolved
Comment on lines +1506 to +1532
[
require.resolve('babel-plugin-ember-modules-api-polyfill'),
{ ignore },
'@embroider/core:babel-plugin-ember-modules-api-polyfill',
],
[
require.resolve('babel-plugin-debug-macros'),
{
flags: [
{
source: '@glimmer/env',
flags: { DEBUG: isDebug, CI: !!process.env.CI },
},
],

externalizeHelpers: {
global: 'Ember',
},

debugTools: {
isDebug,
source: '@ember/debug',
assertPredicateIndex: 1,
},
},
'@embroider/core:@ember/debug stripping',
],
Copy link
Collaborator Author

@stefanpenner stefanpenner Sep 14, 2021

Choose a reason for hiding this comment

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

Now that i re-enabled this block, I remember why I disabled it.

This block makes tests pass, but trips up against the globals warnings. Removing it makes some tests fail, especially when add-ons import things like @glimmer/tracking. There are several approaches we can take to address this, I hope to sync up with @ef4 tomorrow to discuss options.

* changes “real module” mode of ember-source to begin automatically at 4.0.0, rather then 3.27.0-beta.0
* adds emberSourceRealModules option, to opt into this mode prior to 4.0.0
* begins fixing [email protected] issues
* replicate ember-cli-babel’s non-“Real module” mode when the emberSourceRealModules is false
@ef4
Copy link
Contributor

ef4 commented Sep 23, 2021

Closing in favor of #978

@rwjblue rwjblue closed this Sep 27, 2021
@rwjblue
Copy link
Collaborator

rwjblue commented Sep 27, 2021

(I think @ef4 meant to close this with his comment above)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants