Skip to content
This repository has been archived by the owner on Feb 17, 2022. It is now read-only.

Feature: Support Intersection Observer #2

Merged
merged 6 commits into from
Jan 31, 2020
Merged

Feature: Support Intersection Observer #2

merged 6 commits into from
Jan 31, 2020

Conversation

underbyte
Copy link
Contributor

Description

Adding support for intersection observer to nimbuild.

Motivation and Context

  • Devs are requesting this polyfill

How Has This Been Tested?

  • unit tests are updated

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@underbyte underbyte requested a review from neenhouse January 23, 2020 13:43
underbyte
underbyte commented Jan 23, 2020
View reviewed changes
packages/nimbuild-corejs/lib/module-resolver.js Outdated
modules.normal.push('whatwg-fetch');
}

if (!detectIfBrowserHasFeature(targetPlatform, 'intersectionobserver')) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@neenhouse caniuse seems to return false here regardless if the browser is modern or not.

Copy link
Contributor Author

@underbyte underbyte Jan 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Found the issue. caniuse is reporting that in Chrome 65 and up has a side note. So when caniuse-api assert if the feature was supported it only checks for "y" and not "y #4" on this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and now we wait Nyalab/caniuse-api#90

neenhouse
neenhouse reviewed Jan 24, 2020
View reviewed changes
packages/nimbuild-corejs/package-lock.json
@@ -494,19 +494,6 @@
"integrity": "sha512-gCubfBUZ6KxzoibJ+SCUc/57Ms1jz5NjHe4+dI2krNmU5zCPAphyLJYyTOg06ueIyfj+SaCUqmzun7ImlxDcKg==",
"dev": true
},
"@vrbo/nimbuild-webpack": {

This comment was marked as resolved.

Sign in to view

This comment was marked as resolved.

Sign in to view

@neenhouse neenhouse added the enhancement New feature or request label Jan 31, 2020
neenhouse
neenhouse reviewed Jan 31, 2020
View reviewed changes
packages/nimbuild-corejs/lib/index.js
@@ -30,7 +34,10 @@ async function getPolyfillString({
uaString
}) {
// Resolve base feature set based on include / exclude
const features = getBaseFeatureModules({include, exclude});
const features = getBaseFeatureModules({

This comment was marked as resolved.

Sign in to view

neenhouse
neenhouse reviewed Jan 31, 2020
View reviewed changes
packages/nimbuild-corejs/lib/tests/index.test.js
expect(polyfills.entry).toMatchSnapshot();
expect(polyfills.script.length).toBeGreaterThan(15000);

This comment was marked as resolved.

Sign in to view

neenhouse
neenhouse reviewed Jan 31, 2020
View reviewed changes
packages/nimbuild-corejs/lib/utils/isSupported.js Outdated
const browserslist = require("browserslist");
const { features, feature: readableFeature } = require("caniuse-lite");

const featuresList = Object.keys(features);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed? Seems to be unused.

neenhouse
neenhouse reviewed Jan 31, 2020
View reviewed changes
packages/nimbuild-corejs/lib/utils/isSupported.js Outdated
try {
data = readableFeature(features[feature]);
} catch(e) {
throw new ReferenceError(`Please provide a proper feature name. Cannot find ${feature}`);

This comment was marked as resolved.

Sign in to view

neenhouse
neenhouse reviewed Jan 31, 2020
View reviewed changes
packages/nimbuild-corejs/lib/utils/isSupported.js Outdated
});
}

throw new ReferenceError(`browser is an unknown version: ${browsers}`);

This comment was marked as resolved.

Sign in to view

neenhouse
neenhouse reviewed Jan 31, 2020
View reviewed changes
packages/nimbuild-corejs/package.json
"browserslist-useragent": "^3.0.2",
"caniuse-api": "^3.0.0",

This comment was marked as resolved.

Sign in to view

neenhouse
neenhouse suggested changes Jan 31, 2020
View reviewed changes
Copy link
Contributor

@neenhouse neenhouse left a comment

Choose a reason for hiding this comment

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

  • Prettier formatting
  • Code coverage
  • Failing Unit Tests

@neenhouse neenhouse force-pushed the intersection-observer branch from d78bbfc to c82a006 Compare January 31, 2020 16:31
neenhouse
neenhouse previously approved these changes Jan 31, 2020
View reviewed changes
Copy link
Contributor

@neenhouse neenhouse left a comment

Choose a reason for hiding this comment

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

Peer reviewed with @underbyte - pushed PR commit, please confirm @underbyte

@neenhouse neenhouse changed the title Support Intersection Observer Feature: Support Intersection Observer Jan 31, 2020
@underbyte underbyte merged commit 7ab6db9 into master Jan 31, 2020
@underbyte underbyte deleted the intersection-observer branch January 31, 2020 22:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@neenhouse neenhouse neenhouse approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@underbyte @neenhouse