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

configure ocular to build only esm #56

Merged
merged 6 commits into from
Oct 8, 2019
Merged

configure ocular to build only esm #56

merged 6 commits into from
Oct 8, 2019

Conversation

kenns29
Copy link
Collaborator

@kenns29 kenns29 commented Oct 5, 2019

Summary | Related Issue(s)

Addresses #31

Changes

  1. change build script to ocular-clean all && ocular-build --dist esm as the ocular change in tweak build to accept options to build selected targets. uber-web/ocular#251 was published.

Test Plan

Do a test build and publish to see if it works.

Checklist

  • I have made this PR atomic.
  • I have provided enough context for others to review.
  • I have added tests to cover my changes (for features and bug fixes).
  • I have updated the documentation and changelogs accordingly.
  • All new and existing tests passed.

package.json Outdated
@@ -97,7 +97,7 @@
"traverse": "python scripts/traverse.py"
},
"engines": {
"node": ">=8.9.4 <9.0.0",
"node": ">=8.9.4",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

base-ui requires a node engine version greater than v10

Copy link
Contributor

@Firenze11 Firenze11 left a comment

Choose a reason for hiding this comment

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

@kenns29
Copy link
Collaborator Author

kenns29 commented Oct 7, 2019

Please update these proxy files as well https://github.com/uber/manifold/blob/master/modules/manifold/index.js#L1

Did, can you try remove these files and do test publish to see if it works.

@@ -1 +1 @@
module.exports = require('./dist/esm/middleware');
module.exports = require('./dist/middleware');
Copy link
Contributor

Choose a reason for hiding this comment

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

If there's only one build, it should be possible to remove these proxy files entirely. Please evaluate the possibility (trying building, publishing and installing if necessary)

@kenns29 kenns29 merged commit e8155bb into master Oct 8, 2019
@kenns29 kenns29 deleted the ocular branch October 8, 2019 00:24
@Firenze11 Firenze11 mentioned this pull request Nov 5, 2019
5 tasks
Firenze11 added a commit that referenced this pull request Nov 6, 2019
1. Ocular-publish script doesn't work, use lerna publish instead.
2. importing from sub-directories (e,g, `import * from @mlvis/manifold/actions`) still doesn't work despite ocular-build only builds esm version (#56), publish `/dist` folders directly instead.
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.

None yet

3 participants