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

ESM entry points in package.json resolve to an empty file #48

Closed
thescientist13 opened this issue Dec 4, 2021 · 32 comments · Fixed by #50, #54 or #55
Closed

ESM entry points in package.json resolve to an empty file #48

thescientist13 opened this issue Dec 4, 2021 · 32 comments · Fixed by #50, #54 or #55
Labels
bug Something isn't working

Comments

@thescientist13
Copy link
Contributor

thescientist13 commented Dec 4, 2021

Was just testing this out and was debugging why nothing was showing up on the screen, but without any errors and it looks like the ESM / module entry point in package.json ultimately leads to an empty file?

  1. package.json defines a module field (and other fields) that point to a dist/index.js
  2. dist/index.js references file /dist/esm/index.js
  3. /dist/esm/index.js is an empty file 😢

Screen Shot 2021-12-03 at 8 26 10 PM

Screen Shot 2021-12-03 at 8 47 51 PM

Looking forward to giving this a try! ✌️


Somewhat related, but the formal standard to wrangling all these fields is the new export map spec. Might be a nice way to clean all those up, since only main is a standard as far as NodeJS is concerned.

@peterpeterparker
Copy link
Owner

Thanks for the well documented issue 🙏.

PR #50 will solve this. I updated it following the Stencil docs.

I asked a question regarding es2015 and es2017 entries on the Slack channel.
Will merge and release as soon as cleared. Since we are cleaning these entry points, let's do everything correctly 😉.

@peterpeterparker peterpeterparker added the bug Something isn't working label Dec 4, 2021
@thescientist13
Copy link
Contributor Author

Thanks for the quick reply and PR @peterpeterparker !

@peterpeterparker
Copy link
Owner

Alright, we should be good. I released the new version v7.3.0.

Can you check @thescientist13 and let me know if it is alright now?

@thescientist13
Copy link
Contributor Author

thescientist13 commented Dec 5, 2021

Thanks, was able to make some progress! But did encounter another issue.

So now that the module field resolves to dist/custom-elements/index.js, this file references bare modules from the @stencil/core package, that will need to be available and resolvable on the user's system.

import { HTMLElement, createEvent, h, proxyCustomElement } from '@stencil/core/internal/client';
export { setAssetPath, setPlatformOptions } from '@stencil/core/internal/client';

My "quick fix" recommendation for this is to make it explicit by moving @stencil/core out of devDependencies in your package.json and into dependencies so that these

  1. @stencil/core will get installed automatically as part of an npm install when installing this package
  2. ESM based tools can infer the runtime dependency graph of this package, like for building up an import map.

OR

You could make it implicit as a peerDependency in your package.json and have user's manually install prior to consuming this package, and document that in the README.

(My vote would be option 1)

Let me know if you have any questions / need more info. I can open a new issue for this if you prefer. 👍

@peterpeterparker
Copy link
Owner

Thanks for double checking and for the feedback, that's not good, there should be no reference on Stencil.

I following exactly what's in the doc and starter kit, I will investigate further!

In what type of apps and how are you importing the component? Can you provide a sample or tell me a bit more so that I can reproduce the issue and share it too?

Thanks a lot for your help!

@thescientist13
Copy link
Contributor Author

thescientist13 commented Dec 5, 2021

Sure, you can see a PR of my attempts here in my studio website repo, that I'm migrating to use WCs w/ Lit and here is how I am using it.

import { css, html, LitElement, unsafeCSS, TemplateResult } from 'lit';
import { customElement } from 'lit/decorators.js';
import 'web-social-share';

@customElement('app-social-share')
export class SocialShareComponent extends LitElement {

  protected render(): TemplateResult {
    const share = {
      displayNames: true,
      config: [{
        facebook: {
          socialShareUrl: 'https://peterpeterparker.io'
        }
      }, {
        twitter: {
          socialShareUrl: 'https://peterpeterparker.io'
        }
      }]
    };

    return html`
      <div class="container">
        <div class="row">
          <h2 class="header">Interact + Share</h2>
          <web-social-share show="true" share=${share}></web-social-share>
        </div>
      </div>
    `;
  }
} 

Note: I have a couple local modifications for resolving some of the Stencil paths in node_modules to account for ../ relative paths, so bear that in mind for the moment 😊

Using my own project Greenwood to support modern ESM / build + bundle-less workflows for things like:

  • resolving node_modules
  • handling bare module specifiers
  • generating an import map
  • dev server
  • ESM in NodeJS

Similar tools in the space are Snowpack, Vite, and the Modern Web Suite. The Lit docs give a good breakdown of those requirements / expectations for this kind of tooling, so that's what I am trying to support on my end as well. 😃

@peterpeterparker
Copy link
Owner

Thanks a lot for the details, will have a look / try tomorrow or next days.

@peterpeterparker
Copy link
Owner

I fixed the issue and published a new version as a breaking changes since I modified the custom element entry.

Let me know if it works out but, according my test, it should be alright. I was able to import and use the component in a lit starter kit.

Note: it contains some TODO, just limited my self to the bare minimum test ;)

import {LitElement, html, css} from 'lit';
import {customElement, property, query} from 'lit/decorators.js';

import 'web-social-share/dist/components/web-social-share';

@customElement('my-element')
export class MyElement extends LitElement {
  static override styles = css`
    :host {
      display: block;
    }
  `;

  @property({type: Boolean})
  open = false;

  @property({type: Object})
  share = {
    displayNames: true,
    config: [{
      facebook: {
        socialShareUrl: 'https://peterpeterparker.io'
      }
    }, {
      twitter: {
        socialShareUrl: 'https://peterpeterparker.io'
      }
    }]
  };

  @query('web-social-share')
  cmp!: HTMLWebSocialShareElement;

  override render() {
    return html`
      <button @click=${this._onClick}>
        Click
      </button>
      <web-social-share show=${this.open}>
        <span slot="facebook" style="color: #3b5998; font-size: 1.6rem">Facebook</span>
        <span slot="twitter" style="color: #00aced; font-size: 1.6rem">Twitter</span>
      </web-social-share>
      <slot></slot>
    `;
  }

  private _onClick() {
    // TODO: init share object only once in a better way
    this.cmp.share = this.share;

    // TODO: add event listener on cmp event closed to reset open value
    this.open = !this.open;
  }

}

declare global {
  interface HTMLElementTagNameMap {
    'my-element': MyElement;
  }
}

@thescientist13
Copy link
Contributor Author

thescientist13 commented Dec 6, 2021

Thanks, I really appreciate it!

It does looks like all the "ESM" fields in package.json got reverted back to pointing to an empty entry point file again though? (dist -> dist/esm). 😢

I think part of what is contributing to the confusion is that the usage for pulling in the WC itself is not super clear / obvious. Without hints from a package.json, it's not clear how tools / users would know to use this new path from your snippet instead

import 'web-social-share/dist/components/web-social-share';

Which seems to differ from what I see in the stencil docs as recommended by this README, which implies a function call is required?

<script type="module">
  import { defineCustomElements } from '@ionic/core/loader/index.es2017.mjs';
  
  defineCustomElements();
</script>

But i don't see that in your latest example?

Also, it looks like that file still references @stencil/core libs, did that starter kit include Stencil by chance? Or did you install it?


TL;DR I think we are close, I think what is just needed is:

  1. Updated package.json ESM fields to point to dist/components/web-social-share, ex:
     {
       "module": "./dist/components/web-social-share.js"
     }       
  2. Move @stencil/core to the dependency field in package.json (or make it a peerDependency and specify the range / version in the README as part of the npm install step)
  3. Update README with import instructions

Then, when these fields point to the new entry point from your example, NodeJS and tools using those fields as hints should automatically find your entry point. Then either of these variants should work 🙌

import 'web-social-share';

import { xxx } from 'web-social-share';

Thanks again, and sorry for all the trouble. 🥺

@peterpeterparker
Copy link
Owner

The entry points in the package.jsonare now defined as per Stencil documentation and reverted as requested by Stencil's build.

There are different way to consume the component, indeed you can use the loader

import { defineCustomElements } from '@web-social-share/dist/loader';
defineCustomElements();

Or import it:

import 'web-social-share`

or with the custom element

import 'web-social-share/dist/components/web-social-share';

Depends of the app and goals. In any case, it never needs Stencil as a dependency.

Therefore, I think we are close and nothing more need to be done.

As I said in my previous comment, using the last method I was able in a sample project to import and use successfully the lib in a lit element component (see my previous code).

Being said, if you can provide a sample repo that narrow the exact issue, happy to give it a try.

Likewise, PR for the README.md is welcomed.

@thescientist13
Copy link
Contributor Author

thescientist13 commented Dec 6, 2021

Ah, so yeah, looks like these are known issues / open items with Stencil

If i have some time I will try and play around see if if there is a work around to make it work in its current state for ESM based workflows in NodeJS and submit some instructions for that, since as you say, each time you run your build, it will just keeping resolving back to that initial, empty, state.

I will try the loader option as well, to see if explicitly referencing the file as a deep import helps.

@peterpeterparker
Copy link
Owner

Cool! Keep me posted and thx for the help

@thescientist13
Copy link
Contributor Author

thescientist13 commented Dec 14, 2021

Got it working (with a couple tweaks to a fork I made). Left a comment in the Stencil issue, hopefully they will be able to resolve it.

@peterpeterparker
Copy link
Owner

Ah cool 👍

If your tweaks need applies to this component, send me a PR if you do not mind, that would be super.

@thescientist13
Copy link
Contributor Author

thescientist13 commented Dec 15, 2021

Woops, realized I dropped the wrong link in my previous comment. Here was my feedback to the Stencil team in that issue - ionic-team/stencil#2826 (comment)

As an interim solution / work around for anyone curious, I forked the library and changed its module field to point to ./dist/websocialshare/websocialshare.esm.js. As I use import maps for local development, everything was fine, but for production bundling where I use Rollup, because that file imports another file that use variables in dynamic imports, I had to add the dynamic-import-vars plugin, then things worked. 🙌

So, yes, I am totally happy to open a PR, but I guess it is really is up to you because as we see here, everything gets "reset" when you release and run the Stencil CLI so not sure if that's something you would want to "maintain" as it were each time? Plus would want to document the bit about dynamic import vars I suppose.

Let me know though if that all sounds good to you though, happy to upstream what I have here at least if it helps!

@peterpeterparker
Copy link
Owner

Thanks for all the details. Yes agree with you, maybe we should check a bit first to see if something is moving on the Stencil side before merging a new PR. Not against at all, as you said, if it needs to be maintained and needs some documentation, it is maybe worth to "wait a bit" to check if no proper solution comes from the compiler. Ultimately, if not, sure go on with the PR. Does that make sense? Agree with this plan?

@thescientist13
Copy link
Contributor Author

thescientist13 commented Dec 16, 2021

Yeah, I am OK with staying in a holding pattern for now, no rush from my end.

I will probably swing back a little later on anyway to ask some questions on usage / styling in the context of my project and use case, now that I have things working, so will try and preserve your time and support for that. 🙏

Thanks for all your help so far!

@peterpeterparker
Copy link
Owner

Awesome, sounds like a good plan! Thank you

@pranav-js
Copy link

pranav-js commented Dec 20, 2021

I was looking around the best alternative for Lit elements and wanted to try the lazy loading concept of Stencil. But I also see my index.js is pointing to an empty files only :(. Can we take this on priority please, I was doing POC for explaining stencil is best :p and now I can't explain lol.

is there any way I can change entries in the dist folder

@peterpeterparker
Copy link
Owner

peterpeterparker commented Dec 20, 2021

Alright, so first let's reopen this 😉.

@thescientist13 can you provide your modifications as PR, it seems it can be interesting to some.

afterwards and in addition, should we add something in the README?

finally, it the solution is to point the entry in package.json to a different entry point, I think we can submit PRs to the Stencil sample repo and doc, might be useful if it is the effective solutions for everyone.

@pranav-js
Copy link

pranav-js commented Dec 20, 2021

image
oh, what is that different entry point among these inside esm folder? This is pure Stencil starter project btw 😄

@thescientist13
Copy link
Contributor Author

@peterpeterparker

can you provide your modifications as PR, it seems it can be interesting to some.

afterwards and in addition, should we add something in the README?

agree on both counts and will do. Should be able to get that up a little later on tonight for review.

finally, it the solution is to point the entry in package.json to a different entry point, I think we can submit PRs to the Stencil sample repo and doc, might be useful if it is the effective solutions for everyone.

So I see two possible, equally viable options from the Stencil side

  1. Fix it so the right build content actually makes it into the file currently assigned to ESM entry points (dist/esm/index.js)
  2. It should point instead to ./dist/websocialshare/websocialshare.esm.js

I'm not really sure what the right fix is unfortunately, or rather what the intention was from the Stencil team. I'm hoping to use the issue opened in their repo as the main driver of resolution though. 🤞

I myself actually arrived at the file i did by reverse engineering your live demo and seeing that you were using that file, so I tried it locally and it worked. So mostly just trial and error, just to be forthcoming in that regard. 😅


@pranav-js

I believe the simplest solution given the current available artifact in npm, and what I would propose PR is:

  1. Pointing ESM entry points in package.json to a new dist/ files for this package
  2. ESM consumers would need to make sure their build process supported variables in dynamic imports

@peterpeterparker
Copy link
Owner

agree on both counts and will do. Should be able to get that up a little later on tonight for review.

@thescientist13 thanks in advance 🙏

So I see two possible, equally viable options from the Stencil side.

agree with both scenario 1. or 2., make sense 👍. I would say 1. is maybe the clean solution, 2. is the acceptable one, both good in any case. let's keep an eye open and see how it moves forward stencil's side (currently the maintainers are in xmas holidays)

@pranav-js
Copy link

cool, thanks guys, its working now 🤐. stencil needs to update the documentation to point to project_name/project_name.esm.js file or they should do it already. very stupid bug 😂

@peterpeterparker
Copy link
Owner

Wort to notice that npm run build throws such warning if another module entry is used:

[ WARN  ]  Package Json: package.json:6:3
           package.json "module" property is set to
           "dist/websocialshare/websocialshare.esm.js". It's recommended to set
           the "module" property to: ./dist/index.js

      L5:  "main": "dist/index.cjs.js",
      L6:  "module": "dist/websocialshare/websocialshare.esm.js",
      L7:  "es2015": "dist/esm/index.js",

In addition to what we discuss above about documenting Stencil, if the entry point should be modified in Stencil, the warning detection should be updated too.

@peterpeterparker
Copy link
Owner

I have just release @thescientist13 PR in v8.0.1. Thanks!

@pranav-js
Copy link

pranav-js commented Dec 21, 2021

Guys just another help please, its not related to this, but little guidance.

I am having my tsx template in another .js file say

template-three.js has simple onClick which only alerts

import { h } from '@stencil/core';
export const template_three = () => {
  return <button onClick={() => alert()}>Template three here</button>;
};

when I try to call this method my importing in component tsx like this

import { Component, Fragment, h, Host, State } from '@stencil/core';

@Component({
  tag: 'component-two',
  styleUrl: 'component-two.css',
  shadow: true,
})
export class ComponentTwo {
  @State() showComponentOne: any;
  method: any;
  template: string = '';

  constructor() {}

  componentWillRender() {
    this.fetchComp(2);
  }

  fetchComp(type) {
    let this_ = this;
    switch (type) {
      case 0:
        import('./template').then(module => {
          this.showComponentOne = module.template_one;
        });
        break;
      case 1:
        import('./template-two').then(module => {
          this.showComponentOne = module.template_two;
        });
        break;
      case 2:
          import('./template-three').then(module => {
            this.showComponentOne = module.template_three;
          );
        break;
      default:
        break;
    }
  }

  clicked() {
    alert();
  }

  methodHere() {}

  render() {
    let this_ = this;
    return this.showComponentOne ? this.showComponentOne.apply(this) : <div></div>;
  }
}

View renders, but event listners are not working, not even a simple alert :(. When I inspect, I don't see any event attached to button , however if same function I keep inside component class, it works :( !!!

Can you tell me what I am doing wrong in stencil syntax. I can't keep templates in component only cause I have many UI's for same logic.

@thescientist13
Copy link
Contributor Author

thescientist13 commented Dec 21, 2021

@peterpeterparker

Awesome, I will swap out my fork back to this package in a few and report back.


@pranav-js
This probably isn't the right issue for that question, and unfortunately I am not familiar with the Stencil ecosystem. I would probably recommend posting something on StackOverflow or see if the Stencil project has Discussions enabled on its GitHub repo (or maybe has a Discord?). (fwiw, I don't see you using h anywhere in your code, so maybe it's an issue with how you are creating your templates?)

@peterpeterparker
Copy link
Owner

@pranav-js give a try to the Stencil Slack Channel for your question, not sure what you are trying to achieve and it isn't related to this issue, thanks in advance and good luck.

@thescientist13
Copy link
Contributor Author

@peterpeterparker can confirm that pointing back to upstream in my project now, everything is working. 💯

@peterpeterparker
Copy link
Owner

Awesome! Thanks again for the PR and feedback

@pranav-js
Copy link

@thescientist13 hahha, you found my lit comment too for same thing . I was trying lit and stencil together to see which is possible I like stencil but not able to separate out only template part in other file with events attached (view works). same thing in react works though. will have to put some StackOverflow question.

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
3 participants