Add build step and TS support #243

givanse · 2019-01-02T08:21:10Z

Depends on:

README.md

givanse · 2019-01-02T09:19:07Z

package.json

 "description": "Pretender is a mock server library for XMLHttpRequest and Fetch, that comes with an express/sinatra style syntax for defining routes and their handlers.",
 "license": "MIT",
 "engines": {
 "node": ">=4"
 },
 "scripts": {
+ "prepublishOnly": "npm run build && npm run tests-only",


Making sure that the build step is ran before publishing a new version, in case a PR bypassed a red CI somehow.

givanse · 2019-01-02T09:20:34Z

package.json

 "test": "npm run lint && npm run jscs && npm run tests-only",
- "test-ci": "npm run pretest && npm run lint && npm run jscs && npm run tests-only-ci",
+ "test-ci": "npm run pretest && npm run build && npm run lint && npm run jscs && npm run tests-only-ci",


Making sure that the build step is ran before running the tests, in case there is a PR that didn't include the build outputs.

package.json

README.md

mike-north · 2019-01-02T21:13:21Z

src/index.ts

@@ -144,7 +131,7 @@ function Pretender(/* routeMap1, routeMap2, ..., options*/) {
 self.XMLHttpRequest = interceptor(ctx);

 // polyfill fetch when xhr is ready
- this._fetchProps = FakeFetch ? ['fetch', 'Headers', 'Request', 'Response'] : [];


This was just added in a recently merged PR #235, so I'm surprised to see you changing it back. Have you studied the other PR, and verified that your change is correct?

My initial impression was that it was just a fallback put in place to avoid an error state. (had seem to be a reaction to the issue linked to in the original PR)

Now I see that I was wrong, it is about restoring the global space to its original state if needed. I would have used different syntax, but that is out of scope for this PR :)

The thing is, I still don't see how would FakeFetch ever be undefined? Originally, the conversation around it started because of issue #221, but that one is easily explained. The html wasn't loading fetch.umd.js.

I don't think we need the fallback, but I'm going to leave that in place since it isn't immediately pertinent to the objective of this PR.

@xg-wang can you comment here as to the correct behavior? I don't want anyone to roll back a legitimate bugfix

When appearsBrowserified is false, if the whatwg-fetch lib is not installed self.WHATWGFetch will be undefined. This always happens for non ember-cli-pretender users since they have to manual bundle the fetch lib.

In this pr the build output will be:

var FakeFetch = self.WHATWGFetch; var Pretender = (function (self, RouteRecognizer, FakeXMLHttpRequest, FakeFetch) { }(self, RouteRecognizer, FakeXMLHttpRequest, FakeFetch));

The self.WHATWGFetch is pulled in by https:/pretenderjs/pretender/blob/v2.1.1/karma.conf.js#L24

tsconfig.json

src/pretender.es.js

givanse · 2019-01-02T21:40:08Z

@mike-north thanks for the feedback! I'll be addressing each comment.

rollup.config.js

dist/pretender.js

mike-north · 2019-01-03T01:33:11Z

We have to think about how to deal with the type information consumers see, and how this PR may affect/break things. Typically, after TS conversion is complete, hand-written declaration files (*.d.ts) are not necessary anymore (and are in fact a pain to maintain, since they can drift away from the real types in .ts code)

Ultimately (i.e., before we publish a new release) we want type information to come from the .ts source (via a generated .d.ts file) and not from my handwritten definitions (#223). Your TS configuration here is not strict enough to provide real type safety, so here's what I suggest

Leave the index.d.ts in place
Ensure that rollup generates no declaration files (if it does, we can simply remove them in a prepublish step)
Try your branch in a real project, and ensure that type information (via index.d.ts is available to consumers, exactly the same as it is in the latest stable release)

xg-wang · 2019-01-03T06:00:15Z

src/index.ts

- return module.default || module;
-}
-
-var appearsBrowserified = typeof self !== 'undefined' &&


Is this a breaking change?

It shouldn't break anyones app. The build step produces two files with unique consumption mechanisms that immediately inform whether you are browserifying or not depending on which one you choose to use. So, Pretender doesn't need to figure that out at runtime anymore.

givanse · 2019-01-04T20:31:20Z

I linked it to ember-cli-mirage and ran all the tests (npm run test:all), all passed.

Adding the types blew out the diffs. If you prefer, I'm okay to go back to plan B (not generate the declaration file) and postpone the addition of the types for a subsequent PR.

mike-north · 2019-01-04T21:09:28Z

@givanse Yes, let's ensure rollup does not generate declaration files, and stick with the handwritten types until we can get the src/index.ts code up to a level of type safety such that the emitted types will be suitable for consumer use

tsconfig.json

mike-north

this looks good to me now. @givanse we ready to merge?

givanse · 2019-01-04T23:40:15Z

Yeah, it is ready. Thanks for your help.

cowboyd · 2019-02-05T19:31:28Z

This is excellent! Can we get a release based on this version? We're using Pretender in an Angular app and installing off of master fixes the manual module resolution problems that we were encountering 👌

givanse mentioned this pull request Jan 2, 2019

Add a build step and produce ES6 files. #240

Closed

givanse commented Jan 2, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

givanse commented Jan 2, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

givanse force-pushed the add-build-step branch 2 times, most recently from 74fb3dd to 10cf35a Compare January 2, 2019 18:39

mike-north reviewed Jan 2, 2019

View reviewed changes

givanse commented Jan 3, 2019

View reviewed changes

rollup.config.js Outdated Show resolved Hide resolved

mike-north reviewed Jan 3, 2019

View reviewed changes

dist/pretender.js Outdated Show resolved Hide resolved

mike-north self-assigned this Jan 3, 2019

xg-wang reviewed Jan 3, 2019

View reviewed changes

givanse added 4 commits January 4, 2019 13:26

add build step and TypeScript support

9722b25

add module field

1f36273

add .npmignore and prepublishOnly

8d68181

rebase; add tsconfig.json

b2f6df2

givanse force-pushed the add-build-step branch from 2e815f7 to 7da303c Compare January 4, 2019 21:26

givanse commented Jan 4, 2019

View reviewed changes

tsconfig.json Show resolved Hide resolved

PR feedback

eb06984

givanse force-pushed the add-build-step branch from 7da303c to eb06984 Compare January 4, 2019 21:31

mike-north approved these changes Jan 4, 2019

View reviewed changes

mike-north merged commit 8a023ca into pretenderjs:master Jan 4, 2019

givanse deleted the add-build-step branch January 4, 2019 23:51

givanse mentioned this pull request Jan 5, 2019

Import Pretender in Typescript yields empty module #229

Closed

mike-north mentioned this pull request Feb 5, 2019

Restore previous UMD output #247

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add build step and TS support #243

Add build step and TS support #243

givanse commented Jan 2, 2019 •

edited

Loading

givanse Jan 2, 2019

givanse Jan 2, 2019

This comment has been minimized.

mike-north Jan 2, 2019

givanse Jan 2, 2019

givanse Jan 2, 2019

mike-north Jan 2, 2019

xg-wang Jan 3, 2019

givanse commented Jan 2, 2019

mike-north commented Jan 3, 2019 •

edited

Loading

xg-wang Jan 3, 2019

givanse Jan 4, 2019

givanse commented Jan 4, 2019

mike-north commented Jan 4, 2019

mike-north left a comment

givanse commented Jan 4, 2019

cowboyd commented Feb 5, 2019 •

edited

Loading

Add build step and TS support #243

Add build step and TS support #243

Conversation

givanse commented Jan 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

givanse commented Jan 2, 2019

mike-north commented Jan 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

givanse commented Jan 4, 2019

mike-north commented Jan 4, 2019

mike-north left a comment

Choose a reason for hiding this comment

givanse commented Jan 4, 2019

cowboyd commented Feb 5, 2019 • edited Loading

givanse commented Jan 2, 2019 •

edited

Loading

mike-north commented Jan 3, 2019 •

edited

Loading

cowboyd commented Feb 5, 2019 •

edited

Loading