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

handle missed cases for files in exports maps and transient dependency imports #716

Merged

Conversation

thescientist13
Copy link
Member

@thescientist13 thescientist13 commented Sep 6, 2021

Related Issue

resolves #715

Summary of Changes

  1. Update import map if package "flat" export map entry is a file
  2. Add transient dependency imports to import map

TODO

  1. Confirm interop with upgrade website to lit@2 #611 work
  2. Add tests for both cases to develop spec
  3. Validate in Getting Started branches after initial v0.16.0-alpha.0 release, then I will undraft here
  4. Figure out what to do about handle missed cases for files in exports maps and transient dependency imports #716 (comment) - re-opened - leverage NodeJS resolve logic for looking up location of paths for node modules #557 (comment)

@thescientist13 thescientist13 added bug Something isn't working CLI labels Sep 6, 2021
@thescientist13 thescientist13 self-assigned this Sep 6, 2021
@thescientist13 thescientist13 changed the title handle missed cases for flat exports maps and transient imports handle missed cases for files in exports maps and transient dependency imports Sep 6, 2021
.mocharc.js Outdated
@@ -2,5 +2,5 @@ const path = require('path');

module.exports = {
spec: path.join(__dirname, 'packages/**/test/**/**/**/*.spec.js'),
timeout: 30000
timeout: 90000

This comment was marked as outdated.

version "2.4.0"
resolved "https://registry.yarnpkg.com/lit-element/-/lit-element-2.4.0.tgz#b22607a037a8fc08f5a80736dddf7f3f5d401452"
integrity sha512-pBGLglxyhq/Prk2H91nA0KByq/hx/wssJBQFiYqXhGDvEnY31PRGYf1RglVzyLeRysu0IHm2K0P196uLLWmwFg==
dependencies:
lit-html "^1.1.1"

lit-html@^1.1.1:
lit-html@^1.0.0, lit-html@^1.1.1, lit-html@^1.3.0:
Copy link
Member Author

@thescientist13 thescientist13 Sep 10, 2021

Choose a reason for hiding this comment

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

Had to manually patch this since I wanted to keep everything at the 1.3.0 version so as not to disrupt other tests by having to versions of lit-html floating around, even though in theory all dependencies could resolve up to 1.4.1 (current).

For posterity, here is what the original entry looked like before

lit-html@^1.0.0, lit-html@^1.3.0:
  version "1.4.1"
  resolved "https://registry.yarnpkg.com/lit-html/-/lit-html-1.4.1.tgz#0c6f3ee4ad4eb610a49831787f0478ad8e9ae5e0"
  integrity sha512-B9btcSgPYb1q4oSOb/PrOT6Z/H+r6xuNzfH4lFli/AWhYwdtrgQkQWBbIc6mdnf6E2IL3gDXdkkqNktpU0OZQA==

lit-html@^1.1.1:
  version "1.3.0"
  resolved "https://registry.yarnpkg.com/lit-html/-/lit-html-1.3.0.tgz#c80f3cc5793a6dea6c07172be90a70ab20e56034"
  integrity sha512-0Q1bwmaFH9O14vycPHw8C/IeHMk/uSDldVLIefu/kfbTBGIc44KGH6A8p1bDfxUfHdc8q6Ct7kQklWoHgr4t1Q==

And the results of yarn why

% yarn why lit-html
yarn why v1.22.5
[1/4] 🤔  Why do we have the module "lit-html"...?
[2/4] 🚚  Initialising dependency graph...
[3/4] 🔍  Finding dependency...
[4/4] 🚡  Calculating file sizes...
=> Found "[email protected]"
info Has been hoisted to "lit-html"
info Reasons this module exists
   - "workspace-aggregator-4ec7cefd-ead9-47f1-8f99-4c6e0447952a" depends on it
   - Hoisted from "_project_#@greenwood#cli#@lion#button#@lion#core#lit-html"
   - Hoisted from "_project_#@greenwood#cli#@lion#button#@lion#core#@open-wc#scoped-elements#lit-html"
info Disk size without dependencies: "1.31MB"
info Disk size with unique dependencies: "1.31MB"
info Disk size with transitive dependencies: "1.31MB"
info Number of shared dependencies: 0
=> Found "lit-element#[email protected]"
info This module exists because "_project_#@greenwood#cli#lit-element" depends on it.

Yarn resolutions could also be an option too if this ends up being problematic.

},
ExportAllDeclaration(node) {
const sourceValue = node && node.source ? node.source.value : '';

if (sourceValue !== '' && sourceValue.indexOf('.') !== 0 && sourceValue.indexOf('http') !== 0) {
updateImportMap(sourceValue, `/node_modules/${sourceValue}`);
}

if (sourceValue.indexOf('.') === 0 && fs.existsSync(path.join(process.cwd(), 'node_modules', dependency, sourceValue))) {
const moduleContents = fs.readFileSync(path.join(process.cwd(), 'node_modules', dependency, sourceValue));
Copy link
Member Author

@thescientist13 thescientist13 Sep 10, 2021

Choose a reason for hiding this comment

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

In the context of #557 , it seems I did indeed overlook other usages of hardcoded node_modules usages like here, which kind of voids the safety intended by landing #674 . 🤦

In a trivial example like on our home page, npx should work, but if a package.json is introduced, the logic here could have potential to break / behave unexpectedly so just wanted to call this out and maybe reopen #557 ?

@thescientist13 thescientist13 merged commit c4dc5d5 into master Sep 20, 2021
@thescientist13 thescientist13 deleted the bug/issue-715-flat-exports-map-and-transient-imports branch September 20, 2021 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working CLI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

node resolution not handling flat export maps for files and transient dependency imports
1 participant