-
Notifications
You must be signed in to change notification settings - Fork 140
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
Keep app dir #2062
Merged
Merged
Keep app dir #2062
Changes from 1 commit
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
1d07369
keep app dir
ef4 2b939d7
fix app locations in all templates/fixtures
mansona 1627c7e
fix relative imports from addon app-files
mansona a1b48f7
more progress on resolving through package.json exports
ef4 1aa5044
Merge pull request #2063 from embroider-build/fix-keep-app-dir
ef4 380bbe9
Merge remote-tracking branch 'origin/main' into finish-keep-app-dir
mansona 23b927d
update file paths in tests
mansona cb4c171
fix isInComponents check
mansona 1c517cc
bring entrypoint updates to route entrypoints
mansona 2cff656
fix route-split-tests
mansona 9c8979e
update file paths in tests
mansona d850898
follow package.json exports
ef4 0dd42fe
update file paths in tests
mansona 4461cb1
fix resolver test - app resolving requires exports now
mansona e28730f
add exports to audit test
mansona f86c4a9
fix typescript app import
mansona 009d267
add exports to ts-app-template
mansona c159021
update package rules path
mansona 45a2f41
make packageRules.appModules aware of packageJSon.exports
ef4 33eda36
only change appTreeRulesDir for apps, not addons
ef4 2f7acb1
unintentional debugger statement
ef4 ae11588
Merge pull request #2089 from embroider-build/finish-keep-app-dir
ef4 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -466,8 +466,15 @@ export class Resolver { | |
} else { | ||
pkg = requestingPkg; | ||
} | ||
|
||
return logTransition('entrypoint', request, request.virtualize(resolve(pkg.root, '-embroider-entrypoint.js'))); | ||
let matched = resolveExports(pkg.packageJSON, '-embroider-entrypoint.js', { | ||
browser: true, | ||
conditions: ['default', 'imports'], | ||
}); | ||
return logTransition( | ||
'entrypoint', | ||
request, | ||
request.virtualize(resolve(pkg.root, matched?.[0] ?? '-embroider-entrypoint.js')) | ||
); | ||
} | ||
|
||
private handleTestEntrypoint<R extends ModuleRequest>(request: R): R { | ||
|
@@ -494,10 +501,14 @@ export class Resolver { | |
); | ||
} | ||
|
||
let matched = resolveExports(pkg.packageJSON, '-embroider-test-entrypoint.js', { | ||
browser: true, | ||
conditions: ['default', 'imports'], | ||
}); | ||
return logTransition( | ||
'test-entrypoint', | ||
request, | ||
request.virtualize(resolve(pkg.root, '-embroider-test-entrypoint.js')) | ||
request.virtualize(resolve(pkg.root, matched?.[0] ?? '-embroider-test-entrypoint.js')) | ||
); | ||
} | ||
|
||
|
@@ -1333,11 +1344,15 @@ export class Resolver { | |
if (withinEngine) { | ||
// it's a relative import inside an engine (which also means app), which | ||
// means we may need to satisfy the request via app tree merging. | ||
let appJSMatch = await this.searchAppTree( | ||
request, | ||
withinEngine, | ||
explicitRelative(pkg.root, resolve(dirname(request.fromFile), request.specifier)) | ||
); | ||
|
||
let logicalName = engineRelativeName(pkg, resolve(dirname(request.fromFile), request.specifier)); | ||
if (!logicalName) { | ||
return logTransition( | ||
'fallbackResolve: relative failure because this file is not externally accessible', | ||
request | ||
); | ||
} | ||
let appJSMatch = await this.searchAppTree(request, withinEngine, logicalName); | ||
if (appJSMatch) { | ||
return logTransition('fallbackResolve: relative appJsMatch', request, appJSMatch); | ||
} else { | ||
|
@@ -1494,16 +1509,10 @@ export class Resolver { | |
if (engineConfig) { | ||
// we're directly inside an engine, so we're potentially resolvable as a | ||
// global component | ||
|
||
// this kind of mapping is not true in general for all packages, but it | ||
// *is* true for all classical engines (which includes apps) since they | ||
// don't support package.json `exports`. As for a future v2 engine or app: | ||
// this whole method is only relevant for implementing packageRules, which | ||
// should only be for classic stuff. v2 packages should do the right | ||
// things from the beginning and not need packageRules about themselves. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment was claiming we don't care about making this code work in native v2 packages, but it honestly seems easier to just make it work than guarantee that nobody will try to ever use |
||
let inAppName = explicitRelative(engineConfig.root, filename); | ||
|
||
return this.tryReverseComponent(engineConfig.packageName, inAppName); | ||
let inAppName = engineRelativeName(owningPackage, filename); | ||
if (inAppName) { | ||
return this.tryReverseComponent(engineConfig.packageName, inAppName); | ||
} | ||
} | ||
|
||
let engineInfo = this.reverseSearchAppTree(owningPackage, filename); | ||
|
@@ -1555,3 +1564,10 @@ function reliablyResolvable(pkg: V2Package, packageName: string) { | |
function appImportInAppTree(inPackage: Package, inLogicalPackage: Package, importedPackageName: string): boolean { | ||
return inPackage !== inLogicalPackage && importedPackageName === inLogicalPackage.name; | ||
} | ||
|
||
function engineRelativeName(pkg: Package, filename: string): string | undefined { | ||
let outsideName = externalName(pkg.packageJSON, explicitRelative(pkg.root, filename)); | ||
if (outsideName) { | ||
return '.' + outsideName.slice(pkg.name.length); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a first attempt at fixing the way the entrypoint works, since it may need to crawl a different directory than the package root, given what's in package.json exports.
But this is not really sufficient by itself. The
AppFiles
class assumes that app and tests have already been smashed together.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Putting the entrypoint inside a path that is mapped via package.json exports seems like a good idea, so this code is probably helpful. It ensures that we're resolving things that are actually supposed to be resolvable from outside the package. But we can't use only this path to identify which dirs to crawl when building the entrypoint, since there is more than one dir to crawl.
Instead, it probably makes sense to make the crawling code attempt to apply
resolve.exports
tothe-package/tests/example
andthe-package/example
to identify the two directories we need to crawl to build AppFiles.