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

WIP: Attempt to remove the dependencies on fs for browser usage #215

Merged
merged 43 commits into from
Nov 9, 2018

Conversation

jdalrymple
Copy link
Owner

@jdalrymple jdalrymple commented Oct 12, 2018

Summary

fix: Removed xhr library in favour of ky, and switched request for got for a smaller package size and retry functionality #98

breaking: Removed dependency on FS. Now the Projects API takes in two arguments projectId and content as well as an option fileName argument

Testing

  1. Attempt to upload a file to a project https://docs.gitlab.com/ee/api/projects.html#upload-a-file through this library
  2. Test a few other endpoints to make sure the got library replacement worked
  3. Test this library in the browser to see if the replacement through the browser package.json change worked properly using ky instead of got

@jdalrymple jdalrymple changed the title Attempt to remove the dependencies on fs for browser usage WIP: Attempt to remove the dependencies on fs for browser usage Oct 12, 2018
@jdalrymple jdalrymple added this to the 5.0.0 milestone Oct 12, 2018
@jdalrymple jdalrymple mentioned this pull request Oct 12, 2018
Copy link
Contributor

@jetersen jetersen left a comment

Choose a reason for hiding this comment

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

LGTM, nice cleanup 🙌

@jdalrymple
Copy link
Owner Author

Still testing! 😎

@jdalrymple
Copy link
Owner Author

Not sure if im doing the browser bundling correctly:

    "build:browser": "browserify src/index.ts -p [ tsify --noImplicitAny ] > dist/browser.js",

but then i get this error:

> gitlab@4.2.0 build:browser /home/hyde/Projects/Libraries/node-gitlab
> browserify src/index.ts -p [ tsify --noImplicitAny ] > dist/browser.js

TypeScript error: node_modules/@types/node/index.d.ts(6406,21): Error TS2451: Cannot redeclare block-scoped variable 'promisify'.

Any ideas?

… arguments `projectId` and `content` as well as an option fileName argument

fix: Removed xhr library in favour of ky, and switched request for got for a smaller package size and retry functionality
@jdalrymple
Copy link
Owner Author

Finally sorted out the above issue, but ran into another problem. The replacement library Ky, for handling requests has been build to support the latest browsers. I have no issue just supporting the latest browsers for the browser build of this library.

That being said, it will still have to be bundled. If we use the default Ky library, we cannot use browserify since it will only work with es5 modules. This leaves a couple options:

  1. Compile the Ky library down to es5 and use browserify
  2. Don't use browserify, instead use another bundler such as webpack

Thoughts?

@jetersen
Copy link
Contributor

jetersen commented Oct 18, 2018

Don't bother going down the path of compiling Ky and browserify.
Use webpack I could foresee we need it for other bundle options.

Most of electron everything apart from Internet explorer supports es6
So why actually worry about es5 support? Can't the user not polyfill most of it anyway?
https://caniuse.com/#search=es6
https://caniuse.com/#search=Async%20functions

@jdalrymple
Copy link
Owner Author

Yup! Switched to rollup and everything seems to be working except TrySound/rollup-plugin-terser#11. Hopefully i get that sorted soon!

Copy link
Contributor

@jetersen jetersen left a comment

Choose a reason for hiding this comment

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

LGTM so far

src/templates/ResourceAwardEmojis.ts Show resolved Hide resolved
src/infrastructure/RequestHelper.ts Show resolved Hide resolved
src/infrastructure/RequestHelper.ts Show resolved Hide resolved
src/templates/ResourceMembers.ts Show resolved Hide resolved
.gitignore Outdated
@@ -2,3 +2,4 @@ node_modules
npm-debug.log
dist
coverage
.rpt2_cache
Copy link
Contributor

Choose a reason for hiding this comment

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

hmmm maybe .editorconfig would be good to add and set insert final new line. Your making the next edit harder without final new line

Copy link
Owner Author

Choose a reason for hiding this comment

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

Any suggestions for a good editorconfig?

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to apply for the whole repo, of course you can apply different rules for each file type: https://editorconfig.org/

root = true

[*]
end_of_line = lf
charset = utf-8
insert_final_newline = true
ident_style = space
ident_size = 2

.prettierrc.yml Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@jdalrymple
Copy link
Owner Author

Just need to figure out the tests then i can merge this!

@jetersen
Copy link
Contributor

Looks really promising 🙌

Repository owner deleted a comment from jetersen Oct 22, 2018
Repository owner deleted a comment from jetersen Oct 22, 2018
Repository owner deleted a comment from jetersen Oct 22, 2018
@jdalrymple
Copy link
Owner Author

jdalrymple commented Oct 22, 2018

Trying to sort issue with named imports rollup/rollup#2521. We could use { esModuleInterlop: true } in the tsconfig, but im REALLY trying to avoid that if possible.

@jdalrymple
Copy link
Owner Author

jdalrymple commented Oct 29, 2018

Hmm even when i disable treeshaking, the es output is still broken. The top two lines of the output file are:

import * as Request from 'got';
import { get, stream, post, put, delete } from 'got';

I'm not sure why the second line is present since i included the treeshaking: false argument. Ill keep investigating

@jdalrymple
Copy link
Owner Author

Found a way around the treeshaking problem ;)

@jdalrymple
Copy link
Owner Author

Update: My 'genius' idea isn't testable 👎

@jdalrymple
Copy link
Owner Author

@jdalrymple
Copy link
Owner Author

Couldnt get around it, had to include the esModuleInterop

@jdalrymple jdalrymple merged commit ca52057 into next Nov 9, 2018
@jdalrymple jdalrymple deleted the remove-dependency-on-fs branch November 10, 2018 19:40
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.

2 participants