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

Replace dev scripts with ocular-dev-tools #115

Merged
merged 7 commits into from Mar 11, 2019

Conversation

Projects
None yet
2 participants
@Pessimistress
Copy link
Contributor

Pessimistress commented Mar 8, 2019

No description provided.

@Pessimistress Pessimistress requested a review from ibgreen Mar 8, 2019

@Pessimistress Pessimistress force-pushed the ocular-dev-tools branch from 125d178 to 69a3e33 Mar 8, 2019

@@ -9,7 +9,7 @@ import {
} from './read-file-browser';

// read-file-node is excluded from build under browser so don't do indivdual imports
import node from './read-file-node';
import * as node from './read-file-node';

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 8, 2019

Contributor

Ha. Wonder why the test cases didn't catch this? That is very concerning!

This comment has been minimized.

Copy link
@Pessimistress

Pessimistress Mar 8, 2019

Author Contributor

This apparently is ok in es6 but breaks when testing transpiled es5. We have not had any dist test in this repo.

@@ -4,7 +4,7 @@ import {parseFileSync} from '@loaders.gl/core';
import {OBJLoader} from '@loaders.gl/obj';
import {KMLLoader} from '@loaders.gl/kml';

import KML from '@loaders.gl/kml/../data/KML_Samples.kml';
import KML from '../../../kml/data/KML_Samples.kml';

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 8, 2019

Contributor

Why? This is a lot more fragile. I thought we agreed that the alias system was OK as long as we kept it inside loaders.gl?

As far as I can tell it is still used in other files, you just removed it in one place?

This comment has been minimized.

Copy link
@Pessimistress

Pessimistress Mar 8, 2019

Author Contributor

There are two alias systems: one is used with module-alias and automatically created by ocular, which affects all import/require usages. The alias can point to different locations depending on test mode (src or dist).

The second alias system is still in this repo, always point to src, and only affect readFile.

This is the former case, in which using relative path from an alias will break one of the two test modes.

import '@loaders.gl/obj/../test';
import '@loaders.gl/pcd/../test';
import '@loaders.gl/ply/../test';
import "../modules/draco/test";

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 8, 2019

Contributor

Why? I am using this pattern in other repos? Does ocular's aliasing mechanism not support this?

This comment has been minimized.

Copy link
@Pessimistress

Pessimistress Mar 8, 2019

Author Contributor

You cannot use relative path from an alias because it can be mapped to either <module>/src or <module>/dist/es5.

This comment has been minimized.

Copy link
@Pessimistress

Pessimistress Mar 8, 2019

Author Contributor

If this is going to be a regular pattern we use, we can:

  • Add aliases <module_name>/test for all submodules
  • Move data samples into either /modules/**/test/data if they are module-specific or /test/data if they are shared.
Show resolved Hide resolved test/browser.js Outdated
Show resolved Hide resolved babel.config.js Outdated
@ibgreen

ibgreen approved these changes Mar 8, 2019

@Pessimistress Pessimistress force-pushed the ocular-dev-tools branch from 132b119 to d37e768 Mar 11, 2019

@Pessimistress Pessimistress merged commit 5097b18 into master Mar 11, 2019

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@Pessimistress Pessimistress deleted the ocular-dev-tools branch Mar 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.