Skip to content
This repository has been archived by the owner on Jul 30, 2018. It is now read-only.

Adding polyfills #114

Merged
merged 17 commits into from
Nov 10, 2017
Merged

Adding polyfills #114

merged 17 commits into from
Nov 10, 2017

Conversation

rorticus
Copy link
Contributor

@rorticus rorticus commented Sep 11, 2017

Type: feature

The following has been addressed in the PR:

  • There is a related issue
  • All code matches the style guide
  • Unit or Functional tests are included in the PR

Description:

Polyfills (pepjs and intersection-observer) are now loaded in main.

An AMD loader configuration mixin is now provided to make loading polyfills easier. It injects a global function that you call to mix in the polyfill/dojo2 loader configuration with your existing configuration:

shimAmdDependencies({
    baseUrl: '/',
    packages: [ ... ]
});

The intern config for shim has been updated to support this configuration utility (and all loader config has been removed).

Resolves #106

src/main.ts Outdated
import './Symbol';
import has from '@dojo/has/has';

if (has('host-browser')) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kitsonk I couldn't just use the !has() string here because pepjs and intersection-observer error if loaded from node (they expect a browser). I can add the has checks before the require statements if you want?

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 talk about this, because while TypeScript allows it, we don't want to have a require() in the code... We may need to use a dynamic import('pepjs'); instead of the require.


import 'tslib';

global.__values = function (o: any) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to override the typescript __values function because it doesn't handle multibyte strings properly. This will let us use for...of with multibyte strings.

@@ -0,0 +1,16 @@
intern.registerLoader((options) => {
Copy link
Contributor Author

@rorticus rorticus Sep 11, 2017

Choose a reason for hiding this comment

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

Providing our own loader lets us configure and call our shimAmdDependencies method rather than including all the configuration in the intern config. It probably makes sense to have this loader (or something like it) available for other packages to use. Thoughts?

@rorticus rorticus changed the title Issue 106 Adding polyfills Sep 11, 2017
@rorticus rorticus requested a review from pottedmeat September 11, 2017 11:55
return intern.loadScript('node_modules/@dojo/loader/loader.js')
.then(() => intern.loadScript('./_build/src/util/amd.js'))
.then(() => {
(<any> require).config(shimAmdDependencies({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried adding the typings to require. As soon as I import @dojo/interfaces/loader, the file turns into an actual module instead of just a plain function and the whole thing breaks down in agony.

Copy link
Member

Choose a reason for hiding this comment

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

(require as any) and in a test case, the lack of typings is fine, though it would be good to put a comment in the code as to why you are casting.

@dylans dylans added this to the 2017.09 milestone Sep 25, 2017
@dylans dylans modified the milestones: 2017.09, 2017.10 Oct 10, 2017
@rorticus rorticus requested a review from agubler October 10, 2017 13:33
@rorticus
Copy link
Contributor Author

CI failures from typedoc

@rorticus
Copy link
Contributor Author

rorticus commented Oct 10, 2017

Talked to @kitsonk and @matt-gadd , we agreed that we need to,

If JSDOM doesn't solve the problem ,leave out the DOM polyfills for right now.

@rorticus
Copy link
Contributor Author

@kitsonk @matt-gadd Made the changes we talked about!

  • Added browsers module for browsers to import
  • Removed polyfills from @dojo/shim/main
  • Added more tests

@kitsonk
Copy link
Member

kitsonk commented Oct 11, 2017

I believe I am correct in assuming that this will require our tests and build to import '@dojo/shim'; as standard practice?

Copy link
Member

@kitsonk kitsonk left a comment

Choose a reason for hiding this comment

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

We should really update the README with details of these changes as they are fairly significant. Other than that, and a nit, looks 💯

return intern.loadScript('node_modules/@dojo/loader/loader.js')
.then(() => intern.loadScript('./_build/src/util/amd.js'))
.then(() => {
(<any> require).config(shimAmdDependencies({
Copy link
Member

Choose a reason for hiding this comment

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

(require as any) and in a test case, the lack of typings is fine, though it would be good to put a comment in the code as to why you are casting.

@rorticus
Copy link
Contributor Author

@kitsonk , I'll go ahead and update the README with some friendly words. You're correct about needing to import '@dojo/shim'.

@rorticus
Copy link
Contributor Author

rorticus commented Nov 3, 2017

Added a dependency on dojo/grunt-dojo2#169 .

@rorticus rorticus merged commit f2594e2 into dojo:master Nov 10, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants