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

[help needed] Duplicate code cleaning #1944

Closed
wants to merge 6 commits into from
Closed

[help needed] Duplicate code cleaning #1944

wants to merge 6 commits into from

Conversation

zekth
Copy link
Contributor

@zekth zekth commented Mar 16, 2019

I've spotted some duplicates code in js/util.ts which was implemented in deno_std. So exported them in js/test_util.ts like done for some others and cleaned the files calling it.

@zekth
Copy link
Contributor Author

zekth commented Mar 16, 2019

I don't really find why i got those errors. Maybe i'm missing something?

@zekth zekth changed the title Duplicate code cleaning [help needed] Duplicate code cleaning Mar 16, 2019
js/fetch.ts Outdated Show resolved Hide resolved
@kevinkassimo
Copy link
Contributor

kevinkassimo commented Mar 16, 2019

@zekth Did not check too careful before.

I believe duplicates in util.ts and test_util.ts are intended:

util.ts contents are imported and will be bundled into the generated js file for v8 to take a snapshot and goes into the Deno binary. You'll notice that the imports there does not include the .ts extensions.

test_util.ts is just for testing purposes. All these tests are directly run on Deno, so imports with .ts extensions in that file are fine.

However I do think supporting .ts extensions in the build files might be cool. Not so sure if tweaking ts_library_builder would work for this (I'm never a TypeScript expert)

@zekth
Copy link
Contributor Author

zekth commented Mar 16, 2019

@zekth Did not check too careful before.

I believe duplicates in util.ts and test_util.ts are intended:

util.ts contents are imported and will be bundled into the generated js file for v8 to take a snapshot and goes into the Deno binary. You'll notice that the imports there does not include the .ts extensions.

test_util.ts is just for testing purposes. All these tests are directly run on Deno, so imports with .ts extensions in that file are fine.

That's exactly what i just figured out. Closing it, i was totally wrong. Sorry guys.

@zekth zekth closed this Mar 16, 2019
@zekth zekth deleted the dep_clean branch March 16, 2019 23:19
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

2 participants