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

Update publish script #62

Merged
merged 11 commits into from
Nov 6, 2019
Merged

Update publish script #62

merged 11 commits into from
Nov 6, 2019

Conversation

Firenze11
Copy link

@Firenze11 Firenze11 commented Nov 5, 2019

Summary | Related Issue(s)

  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 (configure ocular to build only esm #56), publish /dist folders directly instead.

Testing

Have tested installing in Michelangelo.

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.

@kenns29
Copy link
Collaborator

kenns29 commented Nov 5, 2019

It may be better to publish the dist folder using lerna (lerna/lerna#1282) to fix the subfolder import issue instead of adding back the proxy files.

@gnavvy gnavvy added the not-ready-for-review Still work-in-progress, not ready for review yet. label Nov 5, 2019
v1.1.0-alpha.0

use lerna publish instead of ocular

v1.1.0-alpha.1

use ocular publish

v1.1.0-alpha.2

v1.1.0-alpha.3

v1.1.0-alpha.4

v1.1.0-alpha.4

v1.1.0-alpha.5

v1.1.0-alpha.6

v1.1.0-alpha.7

added public scope

v1.1.0-alpha.8

add public scope

v1.1.0-alpha.9

add public scope

v1.1.0-alpha.10

v1.1.0-alpha.11

v1.1.0-alpha.12

remove root publish script

v1.1.0-alpha.13

change dist/index.js to dist in package.json

v1.1.0-alpha.14

add back proxy files

v1.1.0-alpha.15

remove auto-deploy code

use lerna publish
package.json Outdated
@@ -77,7 +77,7 @@
"test": "jest",
"clean": "ocular-clean all",
"build": "ocular-clean all && ocular-build --dist esm && lerna run build",
"publish": "ocular-publish",
"publish": "lerna publish --force-publish --exact",
Copy link
Author

Choose a reason for hiding this comment

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

This is the main update: use lerna instead of ocular to publish

@@ -0,0 +1 @@
module.exports = require('./dist/actions');
Copy link
Author

Choose a reason for hiding this comment

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

Reverting #56, turns out we still have to use proxy files as long as the code is under /dist directory (non-root directory)

@Firenze11 Firenze11 removed the not-ready-for-review Still work-in-progress, not ready for review yet. label Nov 6, 2019
@Firenze11 Firenze11 changed the title Update publish script and Update publish script and add back proxy files Nov 6, 2019
@Firenze11
Copy link
Author

It may be better to publish the dist folder using lerna (lerna/lerna#1282) to fix the subfolder import issue instead of adding back the proxy files.

Good idea, let me evaluate feasibility

@kenns29
Copy link
Collaborator

kenns29 commented Nov 6, 2019

It may be better to publish the dist folder using lerna (lerna/lerna#1282) to fix the subfolder import issue instead of adding back the proxy files.

Good idea, let me evaluate feasibility

If the lerna approach doesn't work, we can write some publish script, which is quite easy. You can create an issue and assign to me if we take this approach.

"lerna:build": "lerna run build",
"lerna:clean": "lerna run clean",
"lerna:publish": "lerna run publish",
"lerna:publish": "lerna exec cp package.json dist && lerna exec npm publish dist",
Copy link
Author

Choose a reason for hiding this comment

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

Used lerna exec to publish /dist folders only

Copy link
Contributor

Choose a reason for hiding this comment

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

same as above, use yarn publish instead.

@@ -1,12 +1,15 @@
{
"name": "@mlvis/jupyter-graph-builder",
"version": "1.0.0",
"version": "1.1.0-alpha.18",
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we plan to use the alpha version for production usage for now.

@kenns29
Copy link
Collaborator

kenns29 commented Nov 6, 2019

Please also change the PR description if the proxy files are not added back.

@@ -77,17 +76,15 @@
"test": "jest",
"clean": "ocular-clean all",
"build": "ocular-clean all && ocular-build --dist esm && lerna run build",
"publish": "ocular-publish",
"publish": "npm run lerna:publish",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: using yarn instead?

Copy link
Author

@Firenze11 Firenze11 Nov 6, 2019

Choose a reason for hiding this comment

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

I've tried this, but doesn't work -- probably overridden by the default behavior of yarn publish (publishing the root package instead of running a script called "publish")

"lerna:build": "lerna run build",
"lerna:clean": "lerna run clean",
"lerna:publish": "lerna run publish",
"lerna:publish": "lerna exec cp package.json dist && lerna exec npm publish dist",
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above, use yarn publish instead.

@Firenze11 Firenze11 changed the title Update publish script and add back proxy files Update publish script Nov 6, 2019
@Firenze11 Firenze11 merged commit 0aa82a2 into master Nov 6, 2019
@Firenze11 Firenze11 deleted the publish branch November 6, 2019 01:52
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.

3 participants