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

[temporary] additionally builds graph and test into esm #1033

Merged
merged 7 commits into from
Mar 23, 2020

Conversation

ZempTime
Copy link
Contributor

@ZempTime ZempTime commented Feb 28, 2020

Attempt at addressing request for PR from here: #1025

I'm not sure I did this right, so please let me know any changes you'd like. Specifically, here are the areas I think I still need to address:

  • Should I add .web entries for the package.json's files declaration? (Or conversely, remove the web build targets from rollup config?)
  • Verify builds (see below)
  • Changeset

Verifying Builds

Whenever I try to yarn build (or more specifically, tsc), I run into:

src/graph.ts:8:3 - error TS2724: Module ‘“../../../../../../../Users/zempelc/code/node_modules/xstate/lib”’ has no exported member ‘AnyEventObject’. Did you mean ‘DoneEventObject’?
8   AnyEventObject
    ~~~~~~~~~~~~~~
Found 1 error.
error Command failed with exit code 2.

AnyEventObject, however, is definitely there and exported from said directory:

Screen Shot 2020-02-28 at 10 19 51 AM

Which leads me to think I have a misconfiguration somewhere.

When I add xstate as a dep (not a peerDep or devDep), I'm able to build.

@changeset-bot
Copy link

changeset-bot bot commented Feb 28, 2020

🦋 Changeset is good to go

Latest commit: 6e78b3b

We got this.

This PR includes changesets to release 2 packages
Name Type
@xstate/graph Minor
@xstate/test Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Andarist
Copy link
Member

Ill get to reviewing this today/tomorrow, thanks for the PR

@Andarist Andarist self-assigned this Feb 28, 2020
"lib/**/*.d.ts"
"lib/**/*.d.ts",
"es/**/*.js",
"es/**/*.d.ts"
],
"repository": {
Copy link
Contributor Author

@ZempTime ZempTime Feb 28, 2020

Choose a reason for hiding this comment

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

Add/update scripts to:

    "build": "tsc && rollup -c && npm run annotate:es",
    "annotate:es": "babel es --out-dir es --no-babelrc --plugins annotate-pure-calls",

(missed this, noting it)

],
"repository": {
"type": "git",
"url": "git+ssh://git@github.com/davidkpiano/xstate.git"
},
"scripts": {
"clean": "rm -rf dist lib tsconfig.tsbuildinfo",
"build": "tsc",
"build": "tsc && rollup -c && npm run annotate:es",
"annotate:es": "babel es --out-dir es --no-babelrc --plugins annotate-pure-calls",
Copy link
Member

Choose a reason for hiding this comment

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

let's not do this right now, this has been removed on the next branch

name: 'XStateGraph'
}
}),
createUmdConfig({
Copy link
Member

Choose a reason for hiding this comment

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

let's remove those .web builds for now - only xstate has it and I'm still unsure how people actually utilize it, or rather - how they would utilize a build like this which has actual external dependencies (xstate has no dependencies, but this one depends on xstate)

Copy link
Member

@Andarist Andarist left a comment

Choose a reason for hiding this comment

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

Overall looks good, I just have left some minor comments. A little bit sad because the work put into this will be immediately removed as this is already a problem solved on next branch, but well, not much we can do about it as it's counterproductive to port the next setup to master right now.

@ZempTime
Copy link
Contributor Author

ZempTime commented Mar 2, 2020

I understand your feeling that this is seems like bad timing/inefficient effort.

For context, while this is rather inefficient dev-wise, es modules immediately enables ~10+ other devs to start digging into test and graph (this will happen soon as it's there). So I'd like to reinforce, this isn't wasted effort. You're directly helping people be able to use this over the short-term. Additionally, I'll be sharing the PoC with some other communities on top of that, so I'm taking a little responsibility for the very first things they'd do, which is install the deps and look for the esm's.

That said, I'm not married to the pr. In the process of doing this, I figured out how to manually build the packages I needed enough to advance with my PoC, and I could totally compensate for installation issues with communication. I opened the original issue to surface the troubles I ran into, and totally open to whatever way you'd wanna process that. Next question on my mind is: when will next get merged in (thus addressing esm need)? But don't answer that. I never ever wanna ask open-source devs (or any, ideally) for time estimates on principle. That's the question it's up to me to figure out.

To answer that for myself, assuming this is the right place to look at, might be a little bit.

tl;dr - This PR will immediately produce some adoption/exposure for you, so hopefully knowing it's not wasted effort will make it feel less bad. That said, I'm happy to rescind this if you wanna change your mind.

I'll try and have this ready for you by tomorrow!

@davidkpiano
Copy link
Member

@ZempTime This will still be useful for at least a couple months, so thank you! I think it would still be good to merge in for now.

@ZempTime
Copy link
Contributor Author

ZempTime commented Mar 2, 2020

Awesome. Pushed up some changes to hopefully address the remaining comments. Please note:

  • added es dir to yarn clean (forgot it)
  • manually confirmed working builds in both places

Still remaining:

  • Unsure how to address changesets but happy to do so with more direction
  • Hope I addressed the sorted dependencies?

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 2, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 6e78b3b:

Sandbox Source
laughing-cori-pfffs Configuration
wizardly-morse-9hjot Configuration

@ZempTime
Copy link
Contributor Author

ZempTime commented Mar 2, 2020

Final note, idk if this will be a big deal, this new build will produce

packages/xstate-test/es/slimChalk.js

import chalk from 'chalk';

function slimChalk(color, str) {
    return chalk[color](str);
}

export default slimChalk;

I'm not certain that importing chalk from chalk will work, as that doesn't export esm either

@Andarist
Copy link
Member

Andarist commented Mar 2, 2020

This PR will immediately produce some adoption/exposure for you, so hopefully knowing it's not wasted effort will make it feel less bad. That said, I'm happy to rescind this if you wanna change your mind.

I wasn't implying that we shouldn't proceed with that - it was just a general remark that we have solved this already, just on a different branch. As @davidkpiano said - this will still be useful for the v4 release line for a few more months.

Additionally, I'll be sharing the PoC with some other communities on top of that, so I'm taking a little responsibility for the very first things they'd do, which is install the deps and look for the esm's.

Out of curiosity - how do you plan to consume those ESM builds? You still won't be able to load them with correctly with Node and web bundlers (such as webpack) are capable of loading CJS modules.

Next question on my mind is: when will next get merged in (thus addressing esm need)? But don't answer that. I never ever wanna ask open-source devs (or any, ideally) for time estimates on principle.

❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️

I'm not certain that importing chalk from chalk will work, as that doesn't export esm either

Well, this really depends on how exactly do you plan to load those files 😅 As mentioned - would love to learn how do you plan to consume them.

@ZempTime
Copy link
Contributor Author

ZempTime commented Mar 2, 2020

On where I'll be consuming ESM builds:

  • immediate use is here where I'm noobing my way through understanding how to do model-based testing
  • More generalized from my messy repo there, the gold standard is Open WC's development and building guidelines.
  • At work in a custom webpack build/monorepo setup (which I hope to convert to open-wc based at some point)

I need to wrap my head around how to actually implement model-based testing in what I'd loosely label "my ecosystem," and we trend a strange bifurcated mixture of absolute bleeding edge greenfield <--> enterprise needing to deal with every situation ever. Generally, things have settled on "develop using ESM's then build varieties of outputs depending on targets/situation." This is the reason why I'm interested in the es/ outputs.

I personally see model-based testing as an approach really worth investing into. This is totally personal-opinion territory, but I see it as the layer we (the engineering team at work) provides around reused pieces of ui work (think, micro-frontends or composable feature-based frontends) so other business units/clients can consume either our ui 'business logic' layer, and/or our presentation layer. PLUS, have them compose nicely/be self-documenting. We can provide 'spec' surfaces if other teams wanna implement known functionality surfaces using different tech.

I also think it'd be great to model our deployments and drive testing of those through a model-based approach where the pathways are generated by an xstate model, and we move on those manually. I'm not sure what kind of setup xstate will be getting consumed into in that case once this notion moves off of me writing it manually on my computer.

@ZempTime
Copy link
Contributor Author

Any updates on this? I'll be redoubling on model-based testing, soon. 🤗

@Andarist
Copy link
Member

@ZempTime sorry, I was under impression that you were about making some changes to the PR. It seems like not and the PR is green - gonna review this again soon.

@Andarist
Copy link
Member

@ZempTime, in the end, I've ended up using plain tsc to generate those directories as it's a fairly short and simple change.

This might not resolve your issue entirely though, because of chalk being used in @xstate/test. In bundlers, this works thanks to "browser redirects" in package.json, but I'm not sure if your setup is capable of something like this.

@Andarist Andarist merged commit 137b0cd into statelyai:master Mar 23, 2020
@github-actions github-actions bot mentioned this pull request Mar 23, 2020
@github-actions github-actions bot mentioned this pull request Apr 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants