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

Self host types to enable experimentation #3

Merged
merged 4 commits into from
Oct 19, 2020

Conversation

voxpelli
Copy link
Collaborator

This PR is built on top of #2 and adds self-hosted minimal implementations of the relevant types, to enable experimentation with solutions and thus to work towards a solution

@voxpelli
Copy link
Collaborator Author

@fox1t Regarding your feedback on

// Not supported
// import helloWorld from "../../";
// import { helloWorld as namedHelloWorld } from "../../";
// import * as HelloWorldNamespace from "../../";
import helloWorldRequire = require("../../")

It forces people that want to use a legacy TS esm approach to rather than importing it as a esm module import it as a cjs module, that sounds ok to me?

On top of that, import helloWorldRequire = require("../../") works no matter what esModuleInterop is set to with the namespace types, making it easy to move from a non-esModuleInterop setup to a esModuleInterop-setup where one can upgrade to a import later on:

import helloWorld from "../../";
// Not supported
// import { helloWorld as namedHelloWorld } from "../../";
// import * as HelloWorldNamespace from "../../";
import helloWorldRequire = require("../../")

@fox1t
Copy link
Owner

fox1t commented Oct 16, 2020

@voxpelli I agree with you. The problem is the reality of the TS developers today: many doesn't even know anything about import helloWorldRequire = require("../../") and the ones that know don't want to use it.

We were at the point of having more TS and ESM import/export related issues than "real" code issues opened across all of the Fasitfy core repositories. When we decided to make the change and use the "infamous triplet" they just dropped: yours is the first one since months :/

We decided to not force TS users to use import = require syntax.

@fox1t
Copy link
Owner

fox1t commented Oct 17, 2020

@voxpelli can you resolve conflict so we can merge this PR and iterate over it?

I want to change the "selft hosted triplet" to 3 different kind of pacakges:

  1. CJS package with triplet export
  2. TS compiled package
  3. TS compiled package with triplet export
  4. ESM exported package

This will allow us to test all export scenarios in all import scenarios. :)

@voxpelli
Copy link
Collaborator Author

Fixed the conflicts @fox1t, not sure I get what scenarios you want to be added

So right now there are two self-hosted examples:

  • namespace
  • triplet

Each with five test cases:

  • cjs
  • typed-cjs
  • esm
  • typescript with no esModuleInterop
  • typescript with esModuleInterop

Where would you want to add something or change something?

@fox1t
Copy link
Owner

fox1t commented Oct 19, 2020

Nice!

After merging this PR this is the general idea.

Introduction

When I say "package with the triplet" I mean a package that exports:

  1. the infamous triplet in JS code
  2. default and namedExport in TS typings

When I say "package without the triplet" I mean a package that exports:

  1. module.exports = in JS code
  2. namespace export in TS typings

This is the check list on the additions:

  1. make a "fake" node_modules folder and put inside it:
    a. CJS JS package with the triplet
    b. CJS package without the triplet
    c. ESM JS package without the triplet (no triplet is allowed in ESM context)
    d. TS compiled package with the triplet (via custom JS file that adds module.export)
    e. TS compiled pacakge without the triplet (only default and named exports)
    f. TS compiled package that uses export = syntax (implicitly uses namespace types)
  2. adds all of the test
  3. prove that a. and d. are the only packages that are usable in all of the other contexts without issues.

@fox1t fox1t merged commit c4ff2f0 into fox1t:master Oct 19, 2020
@voxpelli voxpelli deleted the self-hosted branch October 19, 2020 16:24
@voxpelli
Copy link
Collaborator Author

@fox1t Right, something you want to do or should I try to find some time to give it a shot?

@fox1t
Copy link
Owner

fox1t commented Oct 19, 2020

I am adding point 1 right now. I am going to open a PR and add you as reviewer ok?

@voxpelli
Copy link
Collaborator Author

@fox1t Totally ok!

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.

2 participants