refactor(deno): remove deno from codebase #3067

rwaskiewicz · 2021-09-16T19:07:56Z

To Folks Who Would Like Deno to be Supported

Please 👍 the summary of the RFC located here. Or leave a comment explaining your use case for Deno (but please do not leave a comment with just '👍')

Pull request checklist

Please check if your PR fulfills the following requirements:

Tests for the changes have been added (for bug fixes / features)
Docs have been reviewed and added / updated if needed (for bug fixes / features)
Build (npm run build) was run locally and any changes were pushed
Unit tests (npm test) were run locally and passed
E2E Tests (npm run test.karma.prod) were run locally and passed
Prettier (npm run prettier) was run locally and passed

Pull request type

Please check the type of change your PR introduces:

What is the current behavior?

We have a good amount of code for Deno, which we sunset recently. It doesn't actually function today.

GitHub Issue Number: N/A

What is the new behavior?

Same as it does today 😆 This just rips Deno out of the Stencil repo

Does this introduce a breaking change?

Yes
No - we've talked about this as a team. Given the fact it does not work today, we do not consider this removal to be a breaking change

Testing

In addition to CI based tests:

Built a tarball with this version of Stencil locally, and was able to use it successfully in a new app spun up with npm init stencil
Smoke tested the output of the test/browser-compiler

Other information

This reverts commit 6d8a61d.

rwaskiewicz · 2021-09-16T19:35:37Z

src/compiler/sys/environment.ts

  typeof global !== 'undefined' &&
  typeof require === 'function' &&
  !!global.process &&
  typeof __filename === 'string' &&
  (!(global as any as Window).origin || typeof (global as any as Window).origin !== 'string');

-export const OS_PLATFORM = IS_NODE_ENV ? process.platform : IS_DENO_ENV ? Deno.build.os : '';


Deno.build.os could return the string "windows" which is why we can safely remove the check for it in the line below (in fact, the compiler yells at us because "windows" is not a valid process.plaform value)

I don't believe "windows" was ever a valid process.platform value, and couldn't find definitive proof of it having existing in https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/node

rwaskiewicz · 2021-09-16T19:38:15Z

src/compiler/sys/modules/os.ts

@@ -2,7 +2,7 @@ import { OS_PLATFORM } from '../environment';

 export const EOL = '\n';

-export const platform = () => (OS_PLATFORM === 'windows' ? 'win32' : OS_PLATFORM);


We're able to do this as a result of this comment's context

bartlomieju · 2021-09-22T20:39:22Z

Hi @rwaskiewicz! I'm one of Deno maintainers. I was just about to open a PR updating vendored Deno dependencies (namely deno.land/[email protected] to deno.land/[email protected]) and found this PR.

I was very impressed how you solved Deno compatiblity using sys pattern and I wanted to use the same pattern in other projects to provide Deno compatibility. FYI we've significantly improved our Node compatibility layer since you started using it.

I'm very curious to hear your thoughts about the subject and problems that you faced, and to know if there's anything we could do to help out with those problems.

rwaskiewicz · 2021-09-24T15:48:06Z

Hey @bartlomieju!

Thanks! Although I can’t take credit for the sys abstraction, it’s a certainly a handy one 🙂

With regards to Deno support in Stencil, we found that our Deno support (which had been marked as experimental by a previous maintainer) wasn’t being actively used. We polled our community, and weren’t able to surface any active users of Stencil + Deno. Additionally, when we tried to understand the current state of the functionality, we realized that it had broken some time ago, and hadn’t been noticed.

The new team behind Stencil is relatively small, and is still getting ramped up on all of its inner workings. While we’d like to support as much as possible, we realized we have to be judicious about the functionality that we support. Due to the lack of usage, we felt that refurbishing Deno support was not going to serve the needs of our community.

With all of that being said, if there is community desire to bring Deno support back, we’re absolutely up for it. That sys abstraction makes adding runtimes rather easy, Also, if you’re interested to hear more/talk further, I’m happy to do so!

bartlomieju · 2021-10-01T09:33:34Z

@rwaskiewicz thanks for the response!

With regards to Deno support in Stencil, we found that our Deno support (which had been marked as experimental by a previous maintainer) wasn’t being actively used. We polled our community, and weren’t able to surface any active users of Stencil + Deno. Additionally, when we tried to understand the current state of the functionality, we realized that it had broken some time ago, and hadn’t been noticed.

The new team behind Stencil is relatively small, and is still getting ramped up on all of its inner workings. While we’d like to support as much as possible, we realized we have to be judicious about the functionality that we support. Due to the lack of usage, we felt that refurbishing Deno support was not going to serve the needs of our community.

That makes perfect sense and I completely understand this situation.

With all of that being said, if there is community desire to bring Deno support back, we’re absolutely up for it. That sys abstraction makes adding runtimes rather easy, Also, if you’re interested to hear more/talk further, I’m happy to do so!

That sounds great; in parallel we're ramping out efforts to make codebases supporting Node and Deno at the same time easier to maintain and hopefully in the coming weeks we'll be able to provide the solution that would lower the maintenance burden.

this commit removes an unnecessary generation of a `dependencies.json` file that would be created at build/release time in the root of the stencil repository. this file was originally used when stencil supported deno; however, support for that runtime was deprecated in #3067. as such, this file and its suppporting infrastructure is no longer needed. there are complimentary functions in the Node sys impl. however, those are no-ops, and need not be called in the cli. we do not remove them at this time for fear of introducing a breaking change. in this commit, we take the one utility function that is used in the build process today and relocated it to be spatially closer to its one caller

rwaskiewicz added 10 commits September 16, 2021 14:48

remove deno cli input

c693d94

rm src/sys/deno

5032a57

Revert "fix(ci): vendor deno for builds (#3020)"

9422d6c

This reverts commit 6d8a61d.

remove deno bundling

9ce00b8

remove deno from enviornment.ts

094e258

remove deno typings

7a187cc

remove deno compiler type from public api

30de945

remove deno reference from sys patching

175275e

final cleanup

e294d90

format the world

f9e4d9d

rwaskiewicz changed the title ~~Rwaskiewicz/rm deno~~ refactor(deno): remove deno from codebase Sep 16, 2021

put up an adr for the changes

0c25ffc

rwaskiewicz force-pushed the rwaskiewicz/rm-deno branch from 37aecc1 to 0c25ffc Compare September 16, 2021 19:32

rwaskiewicz commented Sep 16, 2021

View reviewed changes

rwaskiewicz marked this pull request as ready for review September 16, 2021 19:40

rwaskiewicz requested a review from a team September 16, 2021 19:40

splitinfinities assigned rwaskiewicz Sep 23, 2021

ltm approved these changes Sep 28, 2021

View reviewed changes

Merge branch 'master' into rwaskiewicz/rm-deno

dfc30e9

rwaskiewicz merged commit 037b228 into master Sep 28, 2021

rwaskiewicz deleted the rwaskiewicz/rm-deno branch September 28, 2021 14:12

rwaskiewicz mentioned this pull request Feb 16, 2023

refactor(build): remove dependencies.json generation #4066

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(deno): remove deno from codebase #3067

refactor(deno): remove deno from codebase #3067

rwaskiewicz commented Sep 16, 2021 •

edited

Loading

rwaskiewicz Sep 16, 2021 •

edited

Loading

rwaskiewicz Sep 16, 2021

bartlomieju commented Sep 22, 2021

rwaskiewicz commented Sep 24, 2021

bartlomieju commented Oct 1, 2021

		@@ -2,7 +2,7 @@ import { OS_PLATFORM } from '../environment';

		export const EOL = '\n';

		export const platform = () => (OS_PLATFORM === 'windows' ? 'win32' : OS_PLATFORM);

refactor(deno): remove deno from codebase #3067

refactor(deno): remove deno from codebase #3067

Conversation

rwaskiewicz commented Sep 16, 2021 • edited Loading

To Folks Who Would Like Deno to be Supported

Pull request checklist

Pull request type

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Testing

Other information

rwaskiewicz Sep 16, 2021 • edited Loading

Choose a reason for hiding this comment

rwaskiewicz Sep 16, 2021

Choose a reason for hiding this comment

bartlomieju commented Sep 22, 2021

rwaskiewicz commented Sep 24, 2021

bartlomieju commented Oct 1, 2021

rwaskiewicz commented Sep 16, 2021 •

edited

Loading

rwaskiewicz Sep 16, 2021 •

edited

Loading