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

Addon Dev - Allow ts,gts,gjs files as publicEntrypoints #1106

Merged
merged 1 commit into from
Feb 5, 2022

Conversation

josemarluedke
Copy link
Collaborator

@josemarluedke josemarluedke commented Feb 3, 2022

Closes #1094

For appReexports to work, they need to match the input file name, see .{ts,js}

const globallyAvailable = [
  'components/**/*.{ts,js}',
  'instance-initializers/*.{ts,js}',
  'helpers/**/*.{ts,js}',
];

export default defineConfig({
  output: addon.output(),
  plugins: [
    // .. etc
    addon.publicEntrypoints(['index.ts', ...globallyAvailable]),
    addon.appReexports([...globallyAvailable]),
    // .. etc
  ],
});

@@ -13,7 +13,7 @@ export default function appReexports(opts: {
let pkg = readJsonSync('package.json');
let appJS: Record<string, string> = {};
for (let filename of Object.keys(bundle)) {
if (opts.include.some((glob) => minimatch(filename, glob))) {
if (opts.include.some((glob) => minimatch(filename, glob)) && !minimatch(filename, '**/*.d.ts')) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is to avoid including .d.ts files in app re reexport.

@josemarluedke josemarluedke force-pushed the feat/addon-dev-allow-ts branch from d072b71 to 352567a Compare February 3, 2022 21:42
@@ -13,7 +13,10 @@ export default function appReexports(opts: {
let pkg = readJsonSync('package.json');
let appJS: Record<string, string> = {};
for (let filename of Object.keys(bundle)) {
if (opts.include.some((glob) => minimatch(filename, glob))) {
if (
Copy link
Collaborator

Choose a reason for hiding this comment

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

I kinda like the idea of being able to have .d.ts files co-located with the js and js.map files!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Even for the app files that get's merged into the host app? You can still have colocated in the other files, not in the app folder.

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh, I mean I support this change! I'm excited for it!
I've been making separate types directories because of the way it used to be

NullVoxPopuli added a commit to NullVoxPopuli/ember-statechart-component that referenced this pull request Feb 3, 2022
…but this was orthogonal to the peer dep issue, and I should have tested this on a separate addon
NullVoxPopuli added a commit to NullVoxPopuli/ember-resources that referenced this pull request Feb 3, 2022
@NullVoxPopuli
Copy link
Collaborator

I tested this in ember-resources, and it simplified my config quite a bit. noice

Also tested out @josemarluedke's updates to: https://github.com/NullVoxPopuli/ember-addon-v2-typescript-demo

and things work over there as well!!! (as far as compiling goes anyway!!, @glimmer/component is still undefined somehow

🥳

(I way over-complicated this 😅 )

NullVoxPopuli added a commit to NullVoxPopuli/embroider that referenced this pull request Feb 3, 2022
@@ -2,6 +2,10 @@ import walkSync from 'walk-sync';
import type { Plugin } from 'rollup';
import { join } from 'path';

function normalizeFileExt(fileName: string) {
return fileName.replace(/\.ts|\.gts|\.gjs$/, '.js');
Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli Feb 4, 2022

Choose a reason for hiding this comment

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

tl;dr: I'm back to thinking this PR is fine, and we don't need changes.
It's a positive improvement and if anything definitive comes up, we can do more PRs

🙃


by adding \.hbs to this list of to-replace extensions, I see that my button.hbs is now included in the app-re-exports:
NullVoxPopuli/ember-addon-v2-typescript-demo@93b85af

This is the exact change to your PR

 function normalizeFileExt(fileName: string) {
-  return fileName.replace(/\.ts|\.gts|\.gjs$/, '.js');
+  return fileName.replace(/\.ts|\.gts|\.gjs|\.hbs$/, '.js');
 }

dist/components/demo/button.js now exists, and looks correct:

// ❯ cat dist/components/demo/button.js 
import { setComponentTemplate } from '@ember/component';
import { hbs } from 'ember-cli-htmlbars';

var FlipButton = setComponentTemplate(FlipButton, hbs`<button {{on 'click' @onClick}}>
  flip
</button>
`);

export { FlipButton as default };

and the demo/index.js in dist looks like:

// ❯ cat dist/components/demo/index.js 
import { setComponentTemplate } from '@ember/component';
import TEMPLATE from './index.js';
import { __decorate } from 'tslib';
import { t as tracked, C as Component } from '../../tracked-b2e133b9.js';
import FlipButton from './button.js';
import 'ember-cli-htmlbars';
import '@glimmer/application';

class Demo extends Component {
  constructor(...args) {
    super(...args);
    this.Button = FlipButton;
    this.active = false;

    this.flip = () => this.active = !this.active;
  }

}

__decorate([tracked], Demo.prototype, "active", void 0);

setComponentTemplate(TEMPLATE, Demo);

export { Demo as default };

which is so close to being correct.
Still has that pesky @glimmer/application import.

Copy link
Collaborator

Choose a reason for hiding this comment

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

import TEMPLATE from './index.js';

this seems like it'll break things though 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry for all the noise on the PR -- having a hard time with formatting and nested <details>

For some reason I now have to supply externals, and I think that's breaking a bunch of stuff (lots of stuff to exclude) -- I think addon.dependencies() is supposed to do all this..

In this commit: NullVoxPopuli/ember-addon-v2-typescript-demo@7834ea2

I now get this (with no changes to this PR, #1106):

 cat dist/components/demo/index.js 
import { setComponentTemplate } from '@ember/component';
import { hbs } from 'ember-cli-htmlbars';
import { __decorate } from 'tslib';
import Component from '@glimmer/component';
import { tracked } from '@glimmer/tracking';

var TEMPLATE = setComponentTemplate(TEMPLATE, hbs`Hello there!

<out>{{this.active}}</out>

<this.Button @onClick={{this.flip}} />
`);

var FlipButton = setComponentTemplate(FlipButton, hbs`<button {{on 'click' @onClick}}>
  flip
</button>
`);

class Demo extends Component {
  constructor(...args) {
    super(...args);
    this.Button = FlipButton;
    this.active = false;

    this.flip = () => this.active = !this.active;
  }

}

__decorate([tracked], Demo.prototype, "active", void 0);

setComponentTemplate(TEMPLATE, Demo);

export { Demo as default };

@ef4 ef4 merged commit cb7956f into embroider-build:main Feb 5, 2022
@ef4
Copy link
Contributor

ef4 commented Feb 5, 2022

Nic work, thanks.

@ef4 ef4 added the enhancement New feature or request label Feb 8, 2022
NullVoxPopuli added a commit to NullVoxPopuli/embroider that referenced this pull request Jun 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[addon-dev] Allow TS files in publicEntrypoints / appReexports
3 participants